From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id CF6E83851C25 for ; Thu, 27 Aug 2020 16:45:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CF6E83851C25 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-456-aI_u5rqfON2Db17qLrivmw-1; Thu, 27 Aug 2020 12:45:27 -0400 X-MC-Unique: aI_u5rqfON2Db17qLrivmw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1C56C18BA282; Thu, 27 Aug 2020 16:45:26 +0000 (UTC) Received: from localhost (unknown [10.33.36.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9CE0C78426; Thu, 27 Aug 2020 16:45:25 +0000 (UTC) Date: Thu, 27 Aug 2020 17:45:24 +0100 From: Jonathan Wakely To: Patrick Palka Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: Re: [PATCH] libstdc++: Fix arithmetic bug in chrono::year_month::operator+ Message-ID: <20200827164524.GY3400@redhat.com> References: <20200827152927.1620896-1-ppalka@redhat.com> <20200827155934.GV3400@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Aug 2020 16:45:31 -0000 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(__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 >