Task #3284
closedndn::time::duration should be passed by value
0%
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.
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.
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.
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?
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.
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.