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 Period
s 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 Period
s).
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.