-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replaces magic numbers with compile-time variables #159
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #159 +/- ##
===========================================
- Coverage 64.06% 64.06% -0.01%
===========================================
Files 1079 1079
Lines 55673 55679 +6
Branches 4116 4118 +2
===========================================
+ Hits 35665 35668 +3
- Misses 20008 20011 +3 ☔ View full report in Codecov by Sentry. |
src/eckit/types/Time.h
Outdated
@@ -31,6 +31,12 @@ using Second = double; | |||
|
|||
class Time { | |||
|
|||
public: // types | |||
static constexpr Second secondsInMinute = 60; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that the type Second is right here. IIRC that is a double - and the count of seconds in * is always integral - and is likely to be used in many integral expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used Second
here because DateTime
and Time
use Second (aka double)
e.g., age calculation cases are Second
, while STL uses count time_t (aka long)
so, Second
seemed more sensible to me, and no strong opinions.
happy to change if you suggest another type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DateTime and Time use floating points to represent sub-second values.
time_t usually expresses the number of seconds since epoch.
For these constants (positive numbers expressing the relationship between time units, thus a-dimensional), I would prefer an integer value such as an unsigned long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. there's useconds_t (aka unsigned int)
, not unsigned long
..
any rejection to std::uint64_t
or to template it secondsInMinute<double/uint64_t>
?
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
constexpr secondsInMinute
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
Private downstream CI failed. |
example: seconds in a day would be
eckit::Time::secondsInDay
instead of86400