Project

General

Profile

Actions

Task #2130

closed

Change typeof DEFAULT_INTEREST_LIFETIME

Added by Junxiao Shi about 10 years ago. Updated almost 10 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
0.50 h

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.


Related issues 1 (0 open1 closed)

Blocks NFD - Task #2202: Eliminate DEFAULT_INTEREST_LIFETIME in pit::FaceRecord::updateClosedJunxiao Shi

Actions
Actions #1

Updated by Junxiao Shi almost 10 years ago

  • Blocks Task #2202: Eliminate DEFAULT_INTEREST_LIFETIME in pit::FaceRecord::update added
Actions #2

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.

Actions #3

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.

Actions #4

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.

Actions #5

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.

Actions #6

Updated by Alex Afanasyev almost 10 years ago

There is no semantical difference. Time type is just implementation choice, which doesn't affect anything.

Actions #7

Updated by Junxiao Shi almost 10 years ago

Then documentation (including Doxygen) should represent this as "unspecified duration type".

Actions #8

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.

Actions #9

Updated by Junxiao Shi almost 10 years ago

After changing documentation, is there a reason against declaring as milliseconds?

Actions #10

Updated by Alex Afanasyev almost 10 years ago

What's a point of the change? Keeping as is "saves space"

Actions #11

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.

Actions #12

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.

Actions #13

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.

https://github.com/named-data/ndn-cxx/pull/1

Actions #14

Updated by Junxiao Shi almost 10 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF