On Wed, 28 Oct 2020, Jonathan Wakely wrote:
> On 28/10/20 11:21 -0400, Patrick Palka via Libstdc++ wrote:
> > On Wed, 28 Oct 2020, Patrick Palka wrote:
> >
> > > The conversion function year_month_weekday::operator sys_days computes
> > > the number of days to offset from the first weekday of the month with:
> > >
> > > days{(index()-1)*7}
> > > ^~~~~~~~~~~~~ type 'unsigned'
> > >
> > > We'd like the above to yield -7d when index() is 0u, but our 'days'
> > > alias is based on long instead of int, so the conversion from unsigned
> > > to long instead yields a large positive quantity.
> > >
> > > This patch fixes this by casting the result of index() to int so that
> > > the initializer is sign-extended in the conversion to long. The added
> > > testcase also verifies that we do the right thing when index() == 5.
> > >
> > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk?
> > >
> > > libstdc++-v3/ChangeLog:
> > >
> > > PR libstdc++/96713
> > > * include/std/chrono (year_month_weekday::operator sys_days):
> > > Cast the result of index() to int so that the initializer for
> > > days{} is sign-extended when it's converted to the underlying
> > > type.
> > > * testsuite/std/time/year_month_weekday/3.cc: New test.
> > > ---
> > > libstdc++-v3/include/std/chrono | 3 +-
> > > .../std/time/year_month_weekday/3.cc | 66 +++++++++++++++++++
> > > 2 files changed, 68 insertions(+), 1 deletion(-)
> > > create mode 100644
> > > libstdc++-v3/testsuite/std/time/year_month_weekday/3.cc
> > >
> > > diff --git a/libstdc++-v3/include/std/chrono
> > > b/libstdc++-v3/include/std/chrono
> > > index 7539d7184ea..7c35b78fe59 100644
> > > --- a/libstdc++-v3/include/std/chrono
> > > +++ b/libstdc++-v3/include/std/chrono
> > > @@ -2719,7 +2719,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > operator sys_days() const noexcept
> > > {
> > > auto __d = sys_days{year() / month() / 1};
> > > - return __d + (weekday() - chrono::weekday(__d) + days{(index()-1)*7});
> > > + return __d + (weekday() - chrono::weekday(__d)
> > > + + days{((int)index()-1)*7});
> >
> > On second thought, for consistency with the rest of the header, I guess
> > we should use a functional cast instead of a C-style cast here:
>
> Or static_cast<int>(index()) :-)
>
> Any of those options is OK.
Thanks for the review. I committed the patch just now, albeit with
reference to the unrelated PR96713 instead of PR97613 :( I'll adjust the
ChangeLog entry when it's generated tomorrow.
