[PATCH] libstdc++: Fix operator overload resolution for calendar types

Jonathan Wakely jwakely@redhat.com
Thu Aug 27 16:24:48 GMT 2020


On 25/08/20 15:47 -0400, Patrick Palka via Libstdc++ wrote:
>My original patch that implemented the calendar type operations failed
>to enforce a constraint on some of the addition/subtraction operator
>overloads that take a 'months' argument:
>
>  Constraints: If the argument supplied by the caller for the months
>  parameter is convertible to years, its implicit conversion sequence to
>  years is worse than its implicit conversion sequence to months
>
>This constraint is relevant when adding/subtracting a duration to/from
>say a year_month when the given duration is convertible to both 'months'
>and to 'years'.  The correct behavior here in light of this constraint
>is to select the (more efficient) 'years'-taking overload, but we
>currently emit an ambiguous overload error.
>
>This patch follows the approach taken in the 'date' library and defines
>the constrained 'months'-taking operator overloads as function
>templates, so that we break such a implicit-conversion tie by selecting
>the non-template 'years'-taking overload.
>
>Tested on x86_64-pc-linux-gnu, does this look OK to commit?  (The below
>diff is generated with --ignore-space-change for easier review.  In the
>actual patch, the function templates are indented two extra spaces after
>the template parameter list.)
>
>libstdc++-v3/ChangeLog:
>
>	* include/std/chrono (__detail::__unspecified_month_disambuator):
>	Define.
>	(year_month::operator+=): Turn the 'months'-taking overload
>	into a function template, so that the 'years'-taking overload is
>	selected in case of an equally-ranked implicit conversion
>	sequence to both 'months' and 'years' from the supplied argument.
>	(year_month::operator-=): Likewise.
>	(year_month::operator+): Likewise.
>	(year_month::operator-): Likewise.
>	(year_month_day::operator+=): Likewise.
>	(year_month_day::operator-=): Likewise.
>	(year_month_day::operator+): Likewise.
>	(year_month_day::operator-): Likewise.
>	(year_month_day_last::operator+=): Likewise.
>	(year_month_day_last::operator-=): Likewise.
>	(year_month_day_last::operator+): Likewise
>	(year_month_day_last::operator-): Likewise.
>	(year_month_day_weekday::operator+=): Likewise
>	(year_month_day_weekday::operator-=): Likewise.
>	(year_month_day_weekday::operator+): Likewise.
>	(year_month_day_weekday::operator-): Likewise.
>	(year_month_day_weekday_last::operator+=): Likewise
>	(year_month_day_weekday_last::operator-=): Likewise.
>	(year_month_day_weekday_last::operator+): Likewise.
>	(year_month_day_weekday_last::operator-): Likewise.
>	(testsuite/std/time/year_month/2.cc): New test.
>	(testsuite/std/time/year_month_day/2.cc): New test.
>	(testsuite/std/time/year_month_day_last/2.cc): New test.
>	(testsuite/std/time/year_month_weekday/2.cc): New test.
>	(testsuite/std/time/year_month_weekday_last/2.cc): New test.
>---
> libstdc++-v3/include/std/chrono               | 52 ++++++++++++++++++-
> .../testsuite/std/time/year_month/2.cc        | 40 ++++++++++++++
> .../testsuite/std/time/year_month_day/2.cc    | 40 ++++++++++++++
> .../std/time/year_month_day_last/2.cc         | 40 ++++++++++++++
> .../std/time/year_month_weekday/2.cc          | 40 ++++++++++++++
> .../std/time/year_month_weekday_last/2.cc     | 40 ++++++++++++++
> 6 files changed, 250 insertions(+), 2 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/std/time/year_month/2.cc
> create mode 100644 libstdc++-v3/testsuite/std/time/year_month_day/2.cc
> create mode 100644 libstdc++-v3/testsuite/std/time/year_month_day_last/2.cc
> create mode 100644 libstdc++-v3/testsuite/std/time/year_month_weekday/2.cc
> create mode 100644 libstdc++-v3/testsuite/std/time/year_month_weekday_last/2.cc
>
>diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono
>index 3cc1438a7b6..0e272c3da58 100644
>--- a/libstdc++-v3/include/std/chrono
>+++ b/libstdc++-v3/include/std/chrono
>@@ -2046,6 +2046,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>     // YEAR_MONTH
>
>+    namespace __detail
>+    {
>+      // [time.cal.ym], [time.cal.ymd], etc constrain the 'months'-taking
>+      // addition/subtraction operator overloads like so:
>+      //
>+      //   Constraints: if the argument supplied by the caller for the months
>+      //   parameter is convertible to years, its implicit conversion sequence
>+      //   to years is worse than its implicit conversion sequence to months.
>+      //
>+      // We realize this constraint by defining the 'months'-taking overloads as
>+      // function templates (with a dummy defaulted template parameter), so that
>+      // overload resolution doesn't select the 'months'-taking overload unless
>+      // the implicit conversion sequence to 'months' is better than that to
>+      // 'years'.
>+      using __months_years_conversion_disambiguator = void;
>+    }
>+
>     class year_month
>     {
>     private:
>@@ -2068,6 +2085,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       month() const noexcept
>       { return _M_m; }
>
>+      template<typename = __detail::__months_years_conversion_disambiguator>
> 	constexpr year_month&
> 	operator+=(const months& __dm) noexcept
> 	{

OK for trunk.

I don't really like this solution. It seems like a "clever" hack
rather than a direct expression of the requirement (hence the need to
give it a long descriptive name to say what's going on). But it's
probably better than the alternatives, all things considered.

I considered defining a new type that is implicitly constructible from
both year and month, but does this disambiguation in one place, on its
constructors. Then use a single function taking that type instead of
every pair of overloaded functions taking year and month.

Or adding this in addition to the existing overloads:

   template<typename T>
     requires (convertible_to<T, year> || convertible_to<T, month>)
     constexpr year_month&
     operator+=(const T& t) noexcept
     {
       if constexpr (convertible_to<T, year>)
         return operator+=(static_cast<year>(t));
       else
         return operator+=(static_cast<month>(t));
     }

Or replacing the overload for month with:

   template<convertible_to<month> T>
     requires (!convertible_to<T, year>)
     constexpr year_month&
     operator+=(const T& t) noexcept

But those all probably compile much slower than your solution, and the
new type would have runtime overhead too (it would need to store a
flag to say which type it was constructed with).




More information about the Gcc-patches mailing list