Project

General

Profile

Actions

Task #3284

closed

ndn::time::duration should be passed by value

Added by Davide Pesavento over 9 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Low
Assignee:
-
Category:
Core
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Values of type ndn::time::duration (aka boost::chrono::duration) and its convenience typedefs are never larger than 64 bits. Therefore they should be passed by value, not const reference.

Actions #1

Updated by Alex Afanasyev over 9 years ago

I disagree with this. Time duration is not a typedef, but a non-trivial type. Even though it may be just a number underneath, it is non-obvious.

I would strongly object changing existing code, I would mind using either for the new code.

Actions #2

Updated by Davide Pesavento over 9 years ago

Alex Afanasyev wrote:

I disagree with this. Time duration is not a typedef, but a non-trivial type. Even though it may be just a number underneath, it is non-obvious.

Actually, the ISO C++ standard requires the object representation of chrono::duration to be "an arithmetic type or a class emulating an arithmetic type". So yes, storage-wise it has to be an integral or floating point number of some kind. Plus its constructors must be constexpr.

When I said "convenience typedefs" I was referring to chrono::seconds, chrono::milliseconds, and so on... which are typedefs (again, see the standard).

Moreover, C++14 introduced literal suffixes for them, so they are supposed to be used like literals.

I would strongly object changing existing code, I would mind using either for the new code.

I don't want to change existing code "en masse" either... I actually opened this ticket because Junxiao refused to change from "const&" to "by-value" during code review.

But can we all agree that future code is strongly encouraged to pass these types by value? :) Saying that you don't mind "using either" is like saying that you're ok with code like void foo(const int& x);... which honestly hurts my eyes. So please reconsider.

Actions #3

Updated by Alex Afanasyev over 9 years ago

With a condition that we do not change all the code "en masse", I agree with passing time::duration by value in new code.

Related question. Does this apply to time::Point too or we will keep using time::Point as is?

Actions #4

Updated by Davide Pesavento over 9 years ago

Well, in practice the same applies to chrono::time_point too, since it should have the same size of a duration object. But I don't see any wording in the standard that requires it (I looked very briefly though). I don't think any sane STL implementation would make the type larger though, so I believe it's safe to pass time_point by value too.

In fact, let me revise my position a little: I'm fine with either way of passing an object of type chrono::duration or chrono::time_point, by value or by reference to const. I won't ask to change to one or the other style in code reviews, since they're practically equivalent. Passing by-value looks slightly cleaner (less stuff to write and especially to read), but it's such a minor difference that it doesn't really matter. I hope others agree and don't ask to make useless changes in code reviews.

Actions #5

Updated by Davide Pesavento over 9 years ago

  • Status changed from New to Closed

In any case, there's nothing to change in the code now, so let's close this ticket.

Actions

Also available in: Atom PDF