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

[Patrick Palka <ppalka@redhat.com>]

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.

> +}
> +
>  _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>?

> -- 
> 2.25.1
>