Severity: high (does not compile)
Probability of users hitting this one: low I mean, look, not everyone attempts to use the date library for the temporal dimension of a FFF library
Description: make_time will fail to compile when fed with duration<R, std::ratio<Den, Num>> having both Den and Num greater than 1 (non-trivial ratios).
Config: g++ v 5.4.1 (20160904) on Ubuntu 14.04
Scenario: snippet of code to demonstrate:
using namespace date;
// this is Ok, num is 1
auto alpha=std::chrono::duration<std::uintmax_t, std::ratio<1,5>>{1};
auto alphaTod=make_time(alpha);
// this is Ok, den is one
auto beta=std::chrono::duration<std::uintmax_t, std::ratio<5,1>>{1};
auto betaTod=make_time(beta);
// this is Ok, num is 1 after reducing
auto gamma=std::chrono::duration<std::uintmax_t, std::ratio<3,9>>{1};
auto gammaTod=make_time(gamma);
// this fails to compile
auto delta=std::chrono::duration<std::uintmax_t, std::ratio<2,3>>{1};
auto deltaTod=make_time(delta); // <<<< compilation failure here
// this fails to compile
auto eta=std::chrono::duration<std::uintmax_t, std::ratio<355,113>>{1};
auto etaTod=make_time(eta); // <<<< compilation failure here
Analysis: to be more precise, the bug description should sound like
The detail::classify::subsecond specialization of the time_of_day_storage template will fail to compile when the Period template parameter is represented by a std::ratio which does not divide a second by an integral number
As such:
- the
beta test case is irrelevant to the problem (because there's no subseconds, thus the will be a time_of_day_storage specialization different from the one tagged detail::classify::subsecond
- the
alpha and gamma test cases are equivalent
- only the
delta(subunitary ratio) and eta (supraunitary ratio) test cases are relevant to the failure
The issue is cause by two factors:
- the root cause of the issue lies into the
template <class Duration> struct classify_duration, which is meant to determine the minimum required precision to represent a duration of "a single tick of its Period".
The fault is the failure to note that Periods with supraunitary den will require fractional parts of seconds: because the std::ratio will always be reduced at compile time - see issue #85. If all the multiples of tick-units never show a fractional part, then the ratio::den would be 1. Since it's not, it follows there will be some values that will show fractional seconds - QED.
Once the struct classify_duration is fixed, the compiler will be able to correctly identify which time_of_day_storage specialization to use - and the next bug manifestations will pop up.
- the second factor causing the issue lies in the
detail::classify::subsecond specialization of the time_of_day_storage template, namely the implicit assumption that the arithmetic involving sub_s will always be integral-representable in terms of the same period as the Period passed as the template parameter. To illustrate, please verify if there is an integral number of 355 / 133 ticks (almost-π - see footnote++ at the end) then think what this would do to an operation like sub_s = since_midnight - h_ - m_ - s_ when working with this value for your tick-unit.
Proposed fixes:
The root cause
The fix for the root cause is straight-forward:
template <class Duration>
struct classify_duration
{
static CONSTDATA classify value =
Duration{1} >= days{1} ? classify::not_valid :
Duration::period::den > 1 ? classify::subsecond : // <<< FIX
Duration{1} >= std::chrono::hours{1} ? classify::hour :
Duration{1} >= std::chrono::minutes{1} ? classify::minute :
Duration{1} >= std::chrono::seconds{1} ? classify::second :
classify::subsecond;
};And I argue this fix is necessary. As a preliminary, let's note that the time_of_day struct is more (exclusively?) about user-code-readability/display capabilities than it is about arithmetic; for the time-arithmetic, the duration<R, Precision> is more appropriate.
With 'for-display-purpose' idea in mind, have a look at the code of
template<class CharT, class Traits>
friend
std::basic_ostream<CharT, Traits>&
operator<<(std::basic_ostream<CharT, Traits>& os, const time_of_day_storage& t)around line 3760 of date.h. This code suggests the interpretation of: "Period:den defines the user-required decimal precision for output and, as such, it should be honoured", so the followings will take this interpretation as true
So, for example, if the Period is 478799 / 113 will imply, via that interpretation, that the caller require a precision of milliseconds on output (even if the arithmetic was be done, using the corresponding duration, in ticks a-wee-bit-under-one-hour).
Without the fix, the chosen time_of_day_storage specialization will output an inaccurate time_of_day representation when left-shifted into an ostream
In the 478799 / 113 seconds per tick example, without this fix, the chosen representation will be time_of_day_storage<P, detail::classify::hour>, which will cause the 2 ticks of 478799 / 113 seconds each to be simply displayed as 01 (hours). In fact, under the "Period::den defines precision" interpretation, the millisecond-accurate time-of-day representation would be: 01:59:59.985... or, at its worse precision (of 1h) 02:00:00; but certainly not 01 am.
In conclusion: without this fix, not only that the compiler will baulk at the code using non-trivial ratios for durations (the very cause of me raising the issue), but the classify_duration is incompatible with the user expectation (or, at least, with the expectations one can assume by reading the rest of the date library code)
The fix for the "rational arithmetic of time_of_day when using non-trivial ratio tick units"
This requires a bit more analysis. Of course, something like sub_s_ = round<Precision::period>(since_midnight - h_ - m_ - s_) is a possible and straight-forward solution for the compilation problem (it will satisfy the type constraints immediately and make the compiler happy), but I know for sure that the precision in manipulating (at runtime) time_of_day values expressed in terms of, e.g., almost-π++ tick-units isn't going to be great.
The fault in rationing is subtle (yet simple): the time_of_day_storage in its classify::subsecond chooses to store the hours/mins/secs in something that admits an exact decimal representation of seconds.
And yet the sub_s_ is expressed in terms of the template-arg Period units (see the using precision = std::chrono::duration<Rep, Period>; type alias).
If you still don't get it why it is wrong, think what sub_s_ it is likely to contain if the precision's (non-trivial) ratio is sub-unitary but above 1/2 (testcase delta - ratio of 2/3): any multiple of this tick-unit is going to be captured by the secs_ member, letting the sub_s_ with a value of either 0 or 1 tick count. The matter become worse when the tick-unit becomes supra-unitary (but still non-trivial), like in the testcase eta - huh, the sub_s_ data member will be necessary always-0 (because the secs data member will capture all).
So, what's the solution? Well, the very "Period:den defines the user-required decimal precision" assumption suggest that the sub_s data member be represented not in terms of Period (which may be impossible for supra-unitary periods) but with the finer granularity of ratio<1, Period::den>. True, this will make impossible to honour the <b>precision</b> subseconds() const NOEXCEPT {return sub_s_;} as such, but the precision subseconds() const NOEXCEPT {return round<precision>(sub_s_);}will work (even it will always be zero for supra-unitary Periods).
I argue that the idea of storing sub_s_ in a different (finer than Period) precision is not that unusual, after all we already store the h_, m_ and s_ in a different precision, why sub_s_ should be treated differently?
If we already accept the idea of "internal representation of sub_s_ may be different than the Period template-arg", the next question would be: can we do better at the same price? Well, the purpose being convenience of the representation as time_of_day for the user, storing sub_s_ in units of 1/Period::den is not more advantageous than storing it in decimal units of a certain precision.
So, what decimal representation to choose? Well, no less than 1/Period:den but not unnecessary much finer. so how about
ratio<1, POW10(CEIL_LOG10(Period::den))>?
In short, the proposed fix goes along the idea of:
template <class Rep, class Period>
class time_of_day_storage<std::chrono::duration<Rep, Period>, detail::classify::subsecond>
: private detail::time_of_day_base
{
public:
using precision = std::chrono::duration<Rep, Period>;
private:
// ratio den/num can be signed!!! Who knew?
static CONSTDATA auto DEN=Period::den>0 ? Period::den : -Period::den;
static CONSTDATA auto NUM=Period::num>0 ? Period::num : -Period::num;
static CONSTDATA bool subunit_prec=(NUM<DEN);
using subs_ratio = std::ratio<1, POW10(CEIL_LOG10(DEN))>;
using subs_prec = std::chrono::duration<Rep, subs_ratio>;
using base = detail::time_of_day_base;
std::chrono::minutes m_;
std::chrono::seconds s_;
subs_prec sub_s_;
public:
CONSTCD11 explicit time_of_day_storage(precision since_midnight) NOEXCEPT
: base(std::chrono::duration_cast<std::chrono::hours>(since_midnight), is24hr)
, m_(std::chrono::duration_cast<std::chrono::minutes>(since_midnight - h_))
, s_(std::chrono::duration_cast<std::chrono::seconds>(since_midnight - h_ - m_))
, sub_s_(round<subs_prec>(since_midnight - h_ - m_ - s_))
{}
// maybe SFINAE disable this constructor if precision>1 ??! See parameter.
CONSTCD11 explicit time_of_day_storage(std::chrono::hours h, std::chrono::minutes m,
std::chrono::seconds s,
// maybe disable this constructor if precision>1 ??!
std::enable_if<subunit_prec, precision> sub_s,
unsigned md) NOEXCEPT
: base(h, md)
, m_(m)
, s_(s)
, sub_s_(sub_s)
{}
// ...
CONSTCD11 precision subseconds() const NOEXCEPT {return round<precision>(sub_s_);}
// ...
CONSTCD14 explicit operator precision() const NOEXCEPT
{
return round<precision>(to24hr() + s_ + sub_s_ + m_);
}
//...
++incidentally, the 355 / 133 ratio is the best 6-digit-accurate rational representation of π that can be obtained using fewer than 5 figures when expressing the num and den. As such, it makes a great workhorse for testing the robustness of rational arithmetic.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent