tomaszkam / date

A date and time library based on the C++11/14/17 <chrono> header
Other
0 stars 0 forks source link

[LWG3260] year_month* arithmetic rejects durations convertible to years #39

Closed tomaszkam closed 5 years ago

tomaszkam commented 5 years ago

Arithmetic of year_month/year_month_* struct with durations that are convertible to both years and months (like decades) is ambiguous.

Email thread: [isocpp-lib] year_month* arithmetic rejects durations convertible to years

tomaszkam commented 5 years ago

Tim's Song comment:

It appears that I can't perform arithmetic on year_month* with a duration convertible to months (only), but not with a duration with a unit convertible to years (because such a unit is also convertible to months):

using decades = duration<int, ratio_multiply<ratio<10>, years::period>>; using kilomonths = duration<int, ratio_multiply<ratio<10>, months::period>>; auto ymd = 2001y/1/1; ymd += decades{1}; // error, ambiguous conversion from 'decades' - can be years or months ymd += kilomonths{1}; // OK

This seems unfortunate. Is it intended?

tomaszkam commented 5 years ago

Already implemented: https://github.com/HowardHinnant/date/pull/336 https://github.com/HowardHinnant/date/pull/346

tomaszkam commented 5 years ago

Discussion: Currently, the year_month* types (year_month, year_month_day) provides separate arithmetic operators with duration type of years or months. This is an intentional optimization that avoids performing modulo arithmetic in the former case.

However, these make the arithmetic of year_month* types with durations that are convertible to years (and by consequence to months) ambiguous. For example, following code is ambigous:

using decades = duration, years::period>>;
auto ymd = 2001y/January/1d;
ymd +=  decades(1); // error, ambiguous 

while, less usual durations convertible only convertible to months works correctly:

using decamonths = duration, months::period>>;
auto ymd = 2001y/January/1d;
ymd +=  decamonths(1);

The example implementation resolves the issues by making sure that the years overload will be preferred in case of ambiguity, by making months overload template with default argument for parameters (suggested by Tim Song):

template<class = unspecified>
constexpr year_month_weekday& operator+=(const months& m) noexcept;
constexpr year_month_weekday& operator+=(const years& m) noexcept;

Providing an optimized years overloads may be seen as QoI, as it seems to be covered by as-if rule - the users are no longer allowed to take addresses of free functions provided by the standard unless they are specified as addressable functions.

tomaszkam commented 5 years ago

@HowardHinnant: Do you thin that we should just make a P/R that would change the declaration of months overload to have unspecified template parameter with unspecified value:

template<unspecified = unspecified>
constexpr year_month_weekday& operator+=(const months& m) noexcept;
HowardHinnant commented 5 years ago

Yes, it would be nice to get this fixed. However I'm unsure that we should specify on how to fix it. For example adding a , ... parameter to the months overload might also work.

Perhaps adding to each overview paragraph:

If a duration which is implicitly convertible to both years and months is added or subtracted with year_month, the overload taking years is preferred.

?

HowardHinnant commented 5 years ago

Or perhaps on each overload taking months, add:

Constraints: The argument supplied by the caller is not implicitly convertible to years.

tomaszkam commented 5 years ago

I think that the Constraints version does not really work, as this overload is not really eliminated from overload resolution, it is a worse candidate. I think I will submit issue without wording and then will drop e-mail on LWG reflector asking about the options.

I haven't found any precendence for that kind of wording in standard.

HowardHinnant commented 5 years ago

The Constraints: formulation is the modern equivalent of "...does not participate in overload resolution..."

tomaszkam commented 5 years ago

Yes, and in example implementation, the months overload participate in overload resolution, but is worse candidate. So it does not really do what Constrains impose.

HowardHinnant commented 5 years ago

Is that distinction observable?

HowardHinnant commented 5 years ago

Straight-forward working prototype of the Constraints formulation:

struct year_month
{
    template<class M, class = enable_if_t<is_convertible_v<M, months> &&
                                         !is_convertible_v<M, years>>
            >
        constexpr year_month& operator+=(const M& dm) noexcept;

    constexpr year_month& operator+=(const years& dy) noexcept;
};
HowardHinnant commented 5 years ago

There's probably a neater way to do this with a concept, but the Constraints formulation would describe that as well.

tomaszkam commented 5 years ago

There is no real way to eliminate non-template function from overload resolution. If we would change all months overload to have an template parameter as first argument, then we can say about Constrains on it. But that will block current implementation strategy.

HowardHinnant commented 5 years ago

I was thinking that the standard already has examples of constraints on non-templated functions (requiring a gratuitous invisible template), but I can't find an example of that right now, so perhaps I'm mistaken.

HowardHinnant commented 5 years ago

Default constructor for pair: http://eel.is/c++draft/pairs.pair#5 And this pair constructor: http://eel.is/c++draft/pairs.pair#7 pair move assignment operator: http://eel.is/c++draft/pairs.pair#23 Similarly for tuple: http://eel.is/c++draft/tuple.cnstr#6 optional move constructor: http://eel.is/c++draft/optional.ctor#10

tomaszkam commented 5 years ago

All of them are members of class templates (temploids). I plan too pull this option on LWG reflector and see if people would be happy with it.