Task #2130
closedChange typeof DEFAULT_INTEREST_LIFETIME
100%
Description
ndn::DEFAULT_INTEREST_LIFETIME
currently has time::seconds
type.
It should be changed to time::milliseconds
to be consistent with the definition of InterestLifetime field.
Updated by Junxiao Shi almost 10 years ago
- Blocks Task #2202: Eliminate DEFAULT_INTEREST_LIFETIME in pit::FaceRecord::update added
Updated by Junxiao Shi almost 10 years ago
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
This is a potentially breaking change (in case someone assign this constant to time::seconds
), but I'm equipped with ndn-cxx-breaks now.
Updated by Junxiao Shi almost 10 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
http://gerrit.named-data.net/1461
Notice is sent to ndn-lib and nfd-dev with deadline Nov 26 12:00 UTC.
Updated by Alex Afanasyev almost 10 years ago
I disagree with this issue.
What's a point of this change? The whole idea of using explicit timing in the library is to ensure that everybody knows exactly what time units they are talking about. There is explicitly no indication of time units in the name of DEFAULT_INTEREST_LIFETIME
variable and nobody should have assumed anything about .count()
. If count is used, it must have been used in concert with duration_cast
.
Updated by Junxiao Shi almost 10 years ago
The default value of InterestLifetime should be declared as the same type to be more semantically correct.
Updated by Alex Afanasyev almost 10 years ago
There is no semantical difference. Time type is just implementation choice, which doesn't affect anything.
Updated by Junxiao Shi almost 10 years ago
Then documentation (including Doxygen) should represent this as "unspecified duration type".
Updated by Alex Afanasyev almost 10 years ago
So far, we have only doxygen docs. I'm ok putting a doxygen comment near the constant.
Updated by Junxiao Shi almost 10 years ago
After changing documentation, is there a reason against declaring as milliseconds?
Updated by Alex Afanasyev almost 10 years ago
What's a point of the change? Keeping as is "saves space"
Updated by Junxiao Shi almost 10 years ago
Declaring as a different type requires duration_cast every time it's used.
Hopefully this casting is done at compile time, where no space is saved.
If this casting is done at runtime, that's worse because code execute slower.
Updated by Alex Afanasyev almost 10 years ago
Whenever ".count()" method is used on a time variable, it must be combined with duration_cast
, unless there is a guarantee that the variable represents correct unit.
Anyhow. I don't see the value to do changes. If you really realy want to change, ok then.
Updated by Junxiao Shi almost 10 years ago
- Status changed from Resolved to Code review
Notice deadline has passed. This commit is ready for merging.
Updated by Junxiao Shi almost 10 years ago
- Status changed from Code review to Closed