* [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ @ 2020-08-27 15:29 Patrick Palka 2020-08-27 15:59 ` Jonathan Wakely 0 siblings, 1 reply; 4+ messages in thread From: Patrick Palka @ 2020-08-27 15:29 UTC (permalink / raw) To: gcc-patches; +Cc: libstdc++, Patrick Palka This fixes the months-based addition for year_month when the year_month's month component is zero. Successfully tested on x86_64-pc-linux-gnu, on the 'date' library's calendar and (now) on libcxx's calendar tests. Does this look OK to commit? libstdc++-v3/ChangeLog: * include/std/chrono (year_month::operator+): Properly handle a month value of 0 by casting the month value to int before subtracting 1 from it so that the difference is signed in the subsequent addition. * testsuite/std/time/year_month/1.cc: Test addition of months to a year_month whose month value is below and above the normalized range of [1,12]. --- libstdc++-v3/include/std/chrono | 2 +- libstdc++-v3/testsuite/std/time/year_month/1.cc | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono index 417954d103b..398008c8f31 100644 --- a/libstdc++-v3/include/std/chrono +++ b/libstdc++-v3/include/std/chrono @@ -2133,7 +2133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { // TODO: Optimize? auto __m = __ym.month() + __dm; - auto __i = unsigned{__ym.month()} - 1 + __dm.count(); + auto __i = int(unsigned{__ym.month()}) - 1 + __dm.count(); auto __y = (__i < 0 ? __ym.year() + years{(__i - 11) / 12} : __ym.year() + years{__i / 12}); diff --git a/libstdc++-v3/testsuite/std/time/year_month/1.cc b/libstdc++-v3/testsuite/std/time/year_month/1.cc index 007cfeb2f72..4c331dcdb50 100644 --- a/libstdc++-v3/testsuite/std/time/year_month/1.cc +++ b/libstdc++-v3/testsuite/std/time/year_month/1.cc @@ -83,4 +83,16 @@ constexpr_year_month() static_assert(2017y/33 + months{0} == 2019y/9); static_assert(2010y/January + months{-12} == 2009y/January); + + static_assert(2010y/month{0} + months{-1} == 2009y/November); + static_assert(2010y/month{0} + months{0} == 2009y/December); + static_assert(2010y/month{0} + months{1} == 2010y/January); + static_assert(2010y/month{0} + months{2} == 2010y/February); + static_assert(2010y/month{0} + months{11} == 2010y/November); + static_assert(2010y/month{0} + months{12} == 2010y/December); + static_assert(2010y/month{0} + months{13} == 2011y/January); + + static_assert(months{-1} + 2010y/month{37} == 2012y/December); + static_assert(months{0} + 2010y/month{37} == 2013y/January); + static_assert(months{1} + 2010y/month{37} == 2013y/February); } -- 2.28.0.337.ge9b77c84a0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ 2020-08-27 15:29 [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ Patrick Palka @ 2020-08-27 15:59 ` Jonathan Wakely 2020-08-27 16:37 ` Patrick Palka 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Wakely @ 2020-08-27 15:59 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++ On 27/08/20 11:29 -0400, Patrick Palka via Libstdc++ wrote: >This fixes the months-based addition for year_month when the >year_month's month component is zero. > >Successfully tested on x86_64-pc-linux-gnu, on the 'date' library's >calendar and (now) on libcxx's calendar tests. Does this look OK to >commit? > >libstdc++-v3/ChangeLog: > > * include/std/chrono (year_month::operator+): Properly handle a > month value of 0 by casting the month value to int before > subtracting 1 from it so that the difference is signed in the > subsequent addition. > * testsuite/std/time/year_month/1.cc: Test addition of months to > a year_month whose month value is below and above the normalized > range of [1,12]. >--- > libstdc++-v3/include/std/chrono | 2 +- > libstdc++-v3/testsuite/std/time/year_month/1.cc | 12 ++++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > >diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >index 417954d103b..398008c8f31 100644 >--- a/libstdc++-v3/include/std/chrono >+++ b/libstdc++-v3/include/std/chrono >@@ -2133,7 +2133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > { > // TODO: Optimize? > auto __m = __ym.month() + __dm; >- auto __i = unsigned{__ym.month()} - 1 + __dm.count(); >+ auto __i = int(unsigned{__ym.month()}) - 1 + __dm.count(); The mix of paren-init and braced-init makes me do a double-take. It makes me stop and wonder if there is some reason? Maybe to check for narrowing? But it can't narrow since it just calls year_month::operator unsigned(). But it's really just because int{unsigned{__ym}} /would/ give a narrowing warning, right? Would either int(unsigned(__my)) or static_cast<int>(__ym) make sense instead, do avoid people wondering about it in future? > auto __y = (__i < 0 > ? __ym.year() + years{(__i - 11) / 12} > : __ym.year() + years{__i / 12}); >diff --git a/libstdc++-v3/testsuite/std/time/year_month/1.cc b/libstdc++-v3/testsuite/std/time/year_month/1.cc >index 007cfeb2f72..4c331dcdb50 100644 >--- a/libstdc++-v3/testsuite/std/time/year_month/1.cc >+++ b/libstdc++-v3/testsuite/std/time/year_month/1.cc >@@ -83,4 +83,16 @@ constexpr_year_month() > static_assert(2017y/33 + months{0} == 2019y/9); > > static_assert(2010y/January + months{-12} == 2009y/January); >+ >+ static_assert(2010y/month{0} + months{-1} == 2009y/November); >+ static_assert(2010y/month{0} + months{0} == 2009y/December); >+ static_assert(2010y/month{0} + months{1} == 2010y/January); >+ static_assert(2010y/month{0} + months{2} == 2010y/February); >+ static_assert(2010y/month{0} + months{11} == 2010y/November); >+ static_assert(2010y/month{0} + months{12} == 2010y/December); >+ static_assert(2010y/month{0} + months{13} == 2011y/January); >+ >+ static_assert(months{-1} + 2010y/month{37} == 2012y/December); >+ static_assert(months{0} + 2010y/month{37} == 2013y/January); >+ static_assert(months{1} + 2010y/month{37} == 2013y/February); > } >-- >2.28.0.337.ge9b77c84a0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ 2020-08-27 15:59 ` Jonathan Wakely @ 2020-08-27 16:37 ` Patrick Palka 2020-08-27 16:45 ` Jonathan Wakely 0 siblings, 1 reply; 4+ messages in thread From: Patrick Palka @ 2020-08-27 16:37 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Patrick Palka, gcc-patches, libstdc++ On Thu, 27 Aug 2020, Jonathan Wakely wrote: > On 27/08/20 11:29 -0400, Patrick Palka via Libstdc++ wrote: > > This fixes the months-based addition for year_month when the > > year_month's month component is zero. > > > > Successfully tested on x86_64-pc-linux-gnu, on the 'date' library's > > calendar and (now) on libcxx's calendar tests. Does this look OK to > > commit? > > > > libstdc++-v3/ChangeLog: > > > > * include/std/chrono (year_month::operator+): Properly handle a > > month value of 0 by casting the month value to int before > > subtracting 1 from it so that the difference is signed in the > > subsequent addition. > > * testsuite/std/time/year_month/1.cc: Test addition of months to > > a year_month whose month value is below and above the normalized > > range of [1,12]. > > --- > > libstdc++-v3/include/std/chrono | 2 +- > > libstdc++-v3/testsuite/std/time/year_month/1.cc | 12 ++++++++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/libstdc++-v3/include/std/chrono > > b/libstdc++-v3/include/std/chrono > > index 417954d103b..398008c8f31 100644 > > --- a/libstdc++-v3/include/std/chrono > > +++ b/libstdc++-v3/include/std/chrono > > @@ -2133,7 +2133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > { > > // TODO: Optimize? > > auto __m = __ym.month() + __dm; > > - auto __i = unsigned{__ym.month()} - 1 + __dm.count(); > > + auto __i = int(unsigned{__ym.month()}) - 1 + __dm.count(); > > The mix of paren-init and braced-init makes me do a double-take. > > It makes me stop and wonder if there is some reason? Maybe to check > for narrowing? But it can't narrow since it just calls > year_month::operator unsigned(). [time.cal] seems to mostly use braced-init for converting a calendar type to the underlying type, and I mindlessly followed suit :) e.g. in [time.cal.year.nonmembers] and [time.cal.day.nonmembers]. Ah, but there's [time.cal.month.nonmembers]/7 which uses a static_cast instead. > > But it's really just because int{unsigned{__ym}} /would/ give a > narrowing warning, right? Hmm yes, looks like it. > > Would either int(unsigned(__my)) or static_cast<int>(__ym) make sense > instead, do avoid people wondering about it in future? The first form works for me. The second form is rejected it seems. Does the following look OK to commit? -- >8 -- Subject: [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ This fixes the months-based addition for year_month when the year_month's month component is zero. libstdc++-v3/ChangeLog: * include/std/chrono (year_month::operator+): Properly handle a month value of 0 by casting the month value to int before subtracting 1 from it so that the difference is sign-extended in the subsequent addition. * testsuite/std/time/year_month/1.cc: Test adding months to a year_month whose month component is below or above the normalized range of [1,12]. --- libstdc++-v3/include/std/chrono | 2 +- libstdc++-v3/testsuite/std/time/year_month/1.cc | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono index 417954d103b..f0fa66c7a2b 100644 --- a/libstdc++-v3/include/std/chrono +++ b/libstdc++-v3/include/std/chrono @@ -2133,7 +2133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { // TODO: Optimize? auto __m = __ym.month() + __dm; - auto __i = unsigned{__ym.month()} - 1 + __dm.count(); + auto __i = int(unsigned(__ym.month())) - 1 + __dm.count(); auto __y = (__i < 0 ? __ym.year() + years{(__i - 11) / 12} : __ym.year() + years{__i / 12}); diff --git a/libstdc++-v3/testsuite/std/time/year_month/1.cc b/libstdc++-v3/testsuite/std/time/year_month/1.cc index 007cfeb2f72..4c331dcdb50 100644 --- a/libstdc++-v3/testsuite/std/time/year_month/1.cc +++ b/libstdc++-v3/testsuite/std/time/year_month/1.cc @@ -83,4 +83,16 @@ constexpr_year_month() static_assert(2017y/33 + months{0} == 2019y/9); static_assert(2010y/January + months{-12} == 2009y/January); + + static_assert(2010y/month{0} + months{-1} == 2009y/November); + static_assert(2010y/month{0} + months{0} == 2009y/December); + static_assert(2010y/month{0} + months{1} == 2010y/January); + static_assert(2010y/month{0} + months{2} == 2010y/February); + static_assert(2010y/month{0} + months{11} == 2010y/November); + static_assert(2010y/month{0} + months{12} == 2010y/December); + static_assert(2010y/month{0} + months{13} == 2011y/January); + + static_assert(months{-1} + 2010y/month{37} == 2012y/December); + static_assert(months{0} + 2010y/month{37} == 2013y/January); + static_assert(months{1} + 2010y/month{37} == 2013y/February); } -- 2.28.0.337.ge9b77c84a0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ 2020-08-27 16:37 ` Patrick Palka @ 2020-08-27 16:45 ` Jonathan Wakely 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Wakely @ 2020-08-27 16:45 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++ On 27/08/20 12:37 -0400, Patrick Palka wrote: >On Thu, 27 Aug 2020, Jonathan Wakely wrote: > >> On 27/08/20 11:29 -0400, Patrick Palka via Libstdc++ wrote: >> > This fixes the months-based addition for year_month when the >> > year_month's month component is zero. >> > >> > Successfully tested on x86_64-pc-linux-gnu, on the 'date' library's >> > calendar and (now) on libcxx's calendar tests. Does this look OK to >> > commit? >> > >> > libstdc++-v3/ChangeLog: >> > >> > * include/std/chrono (year_month::operator+): Properly handle a >> > month value of 0 by casting the month value to int before >> > subtracting 1 from it so that the difference is signed in the >> > subsequent addition. >> > * testsuite/std/time/year_month/1.cc: Test addition of months to >> > a year_month whose month value is below and above the normalized >> > range of [1,12]. >> > --- >> > libstdc++-v3/include/std/chrono | 2 +- >> > libstdc++-v3/testsuite/std/time/year_month/1.cc | 12 ++++++++++++ >> > 2 files changed, 13 insertions(+), 1 deletion(-) >> > >> > diff --git a/libstdc++-v3/include/std/chrono >> > b/libstdc++-v3/include/std/chrono >> > index 417954d103b..398008c8f31 100644 >> > --- a/libstdc++-v3/include/std/chrono >> > +++ b/libstdc++-v3/include/std/chrono >> > @@ -2133,7 +2133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> > { >> > // TODO: Optimize? >> > auto __m = __ym.month() + __dm; >> > - auto __i = unsigned{__ym.month()} - 1 + __dm.count(); >> > + auto __i = int(unsigned{__ym.month()}) - 1 + __dm.count(); >> >> The mix of paren-init and braced-init makes me do a double-take. >> >> It makes me stop and wonder if there is some reason? Maybe to check >> for narrowing? But it can't narrow since it just calls >> year_month::operator unsigned(). > >[time.cal] seems to mostly use braced-init for converting a calendar >type to the underlying type, and I mindlessly followed suit :) e.g. in >[time.cal.year.nonmembers] and [time.cal.day.nonmembers]. > >Ah, but there's [time.cal.month.nonmembers]/7 which uses a static_cast >instead. > >> >> But it's really just because int{unsigned{__ym}} /would/ give a >> narrowing warning, right? > >Hmm yes, looks like it. > >> >> Would either int(unsigned(__my)) or static_cast<int>(__ym) make sense >> instead, do avoid people wondering about it in future? > >The first form works for me. The second form is rejected it seems. > >Does the following look OK to commit? OK, thanks. >-- >8 -- > >Subject: [PATCH] libstdc++: Fix arithmetic bug in > chrono::year_month::operator+ > >This fixes the months-based addition for year_month when the >year_month's month component is zero. > >libstdc++-v3/ChangeLog: > > * include/std/chrono (year_month::operator+): Properly handle a > month value of 0 by casting the month value to int before > subtracting 1 from it so that the difference is sign-extended in > the subsequent addition. > * testsuite/std/time/year_month/1.cc: Test adding months to a > year_month whose month component is below or above the > normalized range of [1,12]. >--- > libstdc++-v3/include/std/chrono | 2 +- > libstdc++-v3/testsuite/std/time/year_month/1.cc | 12 ++++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > >diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >index 417954d103b..f0fa66c7a2b 100644 >--- a/libstdc++-v3/include/std/chrono >+++ b/libstdc++-v3/include/std/chrono >@@ -2133,7 +2133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > { > // TODO: Optimize? > auto __m = __ym.month() + __dm; >- auto __i = unsigned{__ym.month()} - 1 + __dm.count(); >+ auto __i = int(unsigned(__ym.month())) - 1 + __dm.count(); > auto __y = (__i < 0 > ? __ym.year() + years{(__i - 11) / 12} > : __ym.year() + years{__i / 12}); >diff --git a/libstdc++-v3/testsuite/std/time/year_month/1.cc b/libstdc++-v3/testsuite/std/time/year_month/1.cc >index 007cfeb2f72..4c331dcdb50 100644 >--- a/libstdc++-v3/testsuite/std/time/year_month/1.cc >+++ b/libstdc++-v3/testsuite/std/time/year_month/1.cc >@@ -83,4 +83,16 @@ constexpr_year_month() > static_assert(2017y/33 + months{0} == 2019y/9); > > static_assert(2010y/January + months{-12} == 2009y/January); >+ >+ static_assert(2010y/month{0} + months{-1} == 2009y/November); >+ static_assert(2010y/month{0} + months{0} == 2009y/December); >+ static_assert(2010y/month{0} + months{1} == 2010y/January); >+ static_assert(2010y/month{0} + months{2} == 2010y/February); >+ static_assert(2010y/month{0} + months{11} == 2010y/November); >+ static_assert(2010y/month{0} + months{12} == 2010y/December); >+ static_assert(2010y/month{0} + months{13} == 2011y/January); >+ >+ static_assert(months{-1} + 2010y/month{37} == 2012y/December); >+ static_assert(months{0} + 2010y/month{37} == 2013y/January); >+ static_assert(months{1} + 2010y/month{37} == 2013y/February); > } >-- >2.28.0.337.ge9b77c84a0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-27 16:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-27 15:29 [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ Patrick Palka 2020-08-27 15:59 ` Jonathan Wakely 2020-08-27 16:37 ` Patrick Palka 2020-08-27 16:45 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).