Re: [PATCH] libstdc++: Fix std::ranges::iota is not included in numeric [PR108760]

[Jonathan Wakely <jwakely@redhat.com>]

On Thu, 18 Apr 2024 at 22:59, Patrick Palka <ppalka@redhat.com> wrote:
>
> On Wed, 17 Apr 2024, Michael Levine (BLOOMBERG/ 919 3RD A) wrote:
>
> > This patch fixes GCC Bug 108760: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108760
> > Before this patch, using std::ranges::iota required including <algorithm> when it should have been sufficient to only include <numeric>.
> >
> > When the patch is applied, the following code will compile: https://godbolt.org/z/33EPeqd1b
> >
> > I added a test case for this change as well.
> >
> > I built my local version of gcc using the following configuration: $ ../gcc/configure --disable-bootstrap --prefix="$(pwd)/_pfx/" --enable-languages=c,c++,lto
> >
> > and I tested my changes by running: $ make check-c++ -jN -k
>
> Nice, thanks for the patch!
>
> >
> > I ran this on the following OS:
> >
> > Virtualization: wsl
> > Operating System: Ubuntu 20.04.6 LTS
> > Kernel: Linux 5.15.146.1-microsoft-standard-WSL2
> > Architecture: x86-64
>
> > From bd04070c281572ed7a3b48e3d33543e25b8c8afe Mon Sep 17 00:00:00 2001
> > From: Michael Levine <mlevine55@bloomberg.net>
> > Date: Fri, 23 Feb 2024 14:13:13 -0500
> > Subject: [PATCH 1/2] Fix the bug
> >
> > Signed-off-by: Michael Levine <mlevine55@bloomberg.net>
> > ---
> >  libstdc++-v3/include/bits/ranges_algo.h | 52 ----------------------
> >  libstdc++-v3/include/bits/stl_numeric.h | 57 ++++++++++++++++++++++++-
> >  2 files changed, 56 insertions(+), 53 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/ranges_algo.h b/libstdc++-v3/include/bits/ranges_algo.h
> > index 62faff173bd..d258be0b93f 100644
> > --- a/libstdc++-v3/include/bits/ranges_algo.h
> > +++ b/libstdc++-v3/include/bits/ranges_algo.h
> > @@ -3521,58 +3521,6 @@ namespace ranges
> >
> >  #endif // __glibcxx_ranges_contains
> >
> > -#if __glibcxx_ranges_iota >= 202202L // C++ >= 23
> > -
> > -  template<typename _Out, typename _Tp>
> > -    struct out_value_result
> > -    {
> > -      [[no_unique_address]] _Out out;
> > -      [[no_unique_address]] _Tp value;
> > -
> > -      template<typename _Out2, typename _Tp2>
> > -     requires convertible_to<const _Out&, _Out2>
> > -       && convertible_to<const _Tp&, _Tp2>
> > -     constexpr
> > -     operator out_value_result<_Out2, _Tp2>() const &
> > -     { return {out, value}; }
> > -
> > -      template<typename _Out2, typename _Tp2>
> > -     requires convertible_to<_Out, _Out2>
> > -       && convertible_to<_Tp, _Tp2>
> > -     constexpr
> > -     operator out_value_result<_Out2, _Tp2>() &&
> > -     { return {std::move(out), std::move(value)}; }
> > -    };
> > -
> > -  template<typename _Out, typename _Tp>
> > -    using iota_result = out_value_result<_Out, _Tp>;
> > -
> > -  struct __iota_fn
> > -  {
> > -    template<input_or_output_iterator _Out, sentinel_for<_Out> _Sent, weakly_incrementable _Tp>
> > -      requires indirectly_writable<_Out, const _Tp&>
> > -      constexpr iota_result<_Out, _Tp>
> > -      operator()(_Out __first, _Sent __last, _Tp __value) const
> > -      {
> > -     while (__first != __last)
> > -       {
> > -         *__first = static_cast<const _Tp&>(__value);
> > -         ++__first;
> > -         ++__value;
> > -       }
> > -     return {std::move(__first), std::move(__value)};
> > -      }
> > -
> > -    template<weakly_incrementable _Tp, output_range<const _Tp&> _Range>
> > -      constexpr iota_result<borrowed_iterator_t<_Range>, _Tp>
> > -      operator()(_Range&& __r, _Tp __value) const
> > -      { return (*this)(ranges::begin(__r), ranges::end(__r), std::move(__value)); }
> > -  };
> > -
> > -  inline constexpr __iota_fn iota{};
> > -
> > -#endif // __glibcxx_ranges_iota
> > -
> >  #if __glibcxx_ranges_find_last >= 202207L // C++ >= 23
> >
> >    struct __find_last_fn
> > diff --git a/libstdc++-v3/include/bits/stl_numeric.h b/libstdc++-v3/include/bits/stl_numeric.h
> > index fe911154ab7..1b06c26dc02 100644
> > --- a/libstdc++-v3/include/bits/stl_numeric.h
> > +++ b/libstdc++-v3/include/bits/stl_numeric.h
> > @@ -59,7 +59,7 @@
> >  #include <bits/concept_check.h>
> >  #include <debug/debug.h>
> >  #include <bits/move.h> // For _GLIBCXX_MOVE
> > -
> > +#include <bits/ranges_base.h> // For _Range as used by std::ranges::iota
> >
> >  namespace std _GLIBCXX_VISIBILITY(default)
> >  {
> > @@ -102,6 +102,61 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >      }
> >  #endif
> >
> > +namespace ranges
> > +{
> > +#if __glibcxx_ranges_iota >= 202202L // C++ >= 23
> > +
> > +  template<typename _Out, typename _Tp>
> > +    struct out_value_result
> > +    {
> > +      [[no_unique_address]] _Out out;
> > +      [[no_unique_address]] _Tp value;
> > +
> > +      template<typename _Out2, typename _Tp2>
> > +     requires convertible_to<const _Out&, _Out2>
> > +       && convertible_to<const _Tp&, _Tp2>
> > +     constexpr
> > +     operator out_value_result<_Out2, _Tp2>() const &
> > +     { return {out, value}; }
> > +
> > +      template<typename _Out2, typename _Tp2>
> > +     requires convertible_to<_Out, _Out2>
> > +       && convertible_to<_Tp, _Tp2>
> > +     constexpr
> > +     operator out_value_result<_Out2, _Tp2>() &&
> > +     { return {std::move(out), std::move(value)}; }
> > +    };
>
> IIUC out_value_result should continue to be available from <algorithm>, so we
> probably don't want to move it to <numeric> (or one of its internal headers).
> Better would be to move it to <bits/ranges_algobase.h> I think.
>
> > +
> > +  template<typename _Out, typename _Tp>
> > +    using iota_result = out_value_result<_Out, _Tp>;
> > +
> > +  struct __iota_fn
> > +  {
> > +    template<input_or_output_iterator _Out, sentinel_for<_Out> _Sent, weakly_incrementable _Tp>
> > +      requires indirectly_writable<_Out, const _Tp&>
> > +      constexpr iota_result<_Out, _Tp>
> > +      operator()(_Out __first, _Sent __last, _Tp __value) const
> > +      {
> > +     while (__first != __last)
> > +       {
> > +         *__first = static_cast<const _Tp&>(__value);
> > +         ++__first;
> > +         ++__value;
> > +       }
> > +     return {std::move(__first), std::move(__value)};
> > +      }
> > +
> > +    template<weakly_incrementable _Tp, output_range<const _Tp&> _Range>
> > +      constexpr iota_result<borrowed_iterator_t<_Range>, _Tp>
> > +      operator()(_Range&& __r, _Tp __value) const
> > +      { return (*this)(ranges::begin(__r), ranges::end(__r), std::move(__value)); }
> > +  };
> > +
> > +  inline constexpr __iota_fn iota{};
> > +
> > +#endif // __glibcxx_ranges_iota
>
> And it might be cleaner to move ranges::iota directly into <numeric>
> instead (and then include <bits/ranges_algobase.h> directly from <numeric>
> to bring in <bits/ranges_base.h> and out_value_result) since
> <bits/stl_numeric.h> has other users besides <numeric> which would
> otherwise get unnecessarily bloated.

Good idea, thanks, Patrick.

> > +}
> > +
> >  _GLIBCXX_END_NAMESPACE_VERSION
> >
> >  _GLIBCXX_BEGIN_NAMESPACE_ALGO
> > --
> > 2.25.1
> >
> >
> > From c52c8d79fb3c7dc9d2512d1635712ffcd3dea07c Mon Sep 17 00:00:00 2001
> > From: Michael Levine <mlevine55@bloomberg.net>
> > Date: Tue, 16 Apr 2024 16:45:37 -0400
> > Subject: [PATCH 2/2] Added a test to verify that the bug has been fixed
> >
> > Signed-off-by: Michael Levine <mlevine55@bloomberg.net>
> > ---
> >  .../testsuite/std/ranges/iota/108760.cc       | 41 +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 libstdc++-v3/testsuite/std/ranges/iota/108760.cc
> >
> > diff --git a/libstdc++-v3/testsuite/std/ranges/iota/108760.cc b/libstdc++-v3/testsuite/std/ranges/iota/108760.cc
> > new file mode 100644
> > index 00000000000..4f71383687c
> > --- /dev/null
> > +++ b/libstdc++-v3/testsuite/std/ranges/iota/108760.cc
> > @@ -0,0 +1,41 @@
> > +// Copyright (C) 2020-2024 Free Software Foundation, Inc.
> > +//
> > +// This file is part of the GNU ISO C++ Library.  This library is free
> > +// software; you can redistribute it and/or modify it under the
> > +// terms of the GNU General Public License as published by the
> > +// Free Software Foundation; either version 3, or (at your option)
> > +// any later version.
> > +
> > +// This library is distributed in the hope that it will be useful,
> > +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +// GNU General Public License for more details.
> > +
> > +// You should have received a copy of the GNU General Public License along
> > +// with this library; see the file COPYING3.  If not see
> > +// <http://www.gnu.org/licenses/>.
> > +
> > +// Fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108760
> > +// { dg-do run { target c++23 } }
> > +
> > +#include <numeric>
> > +#include <testsuite_hooks.h>
> > +
> > +const int ARR_SIZE = 4;
> > +
> > +void
> > +test01()
> > +{
> > +    int expected_arr[ARR_SIZE] = {0, 1, 2, 3};
> > +    int input_arr[ARR_SIZE] = {0, 0, 0, 0};
> > +    std::ranges::iota(input_arr, 0);
> > +    for (int i = 0; i < ARR_SIZE; i++) {
> > +        VERIFY( input_arr[i] == expected_arr[i]);
> > +    }
> > +}
> > +
> > +int
> > +main()
> > +{
> > +    test01();
> > +}
>
> Instead of adding a new test, perhaps we should just move the
> existing test libstdc++-v3/testsuite/25_algorithms/iota/1.cc
> to libstdc++-v3/testsuite/26_numerics/iota/1.cc and change it
> to include <numeric> instead of <algorithm>?


Yes please, the existing test is better as it uses an output range
instead of a contiguous range for the result. And the new test has
incorrect copyright/licence info. As a new test, it should not have
2020-2024 as the copyright date, and it should not be copyright FSF
unless Michael (or Bloomberg) has a copyright assignment on file to
assign copyright to the FSF. Since Michael added a DCO Signed-off-by:
tag, I assume this is being contributed under the
https://gcc.gnu.org/dco.html terms, not under copyright assignment, so
the test should not be copyright FSF at all.

I don't bother adding copyright and license info to trivial tests like
this, it's not doing anything meaningful and so I don't consider it
copyrightable. There's no novel implementation or non-trivial logic in
this test code, it's literally just exercising an API for the purpose
of testing it. There is not really any other way to do that, so there
is no "originality" in that work.