Re: [PATCH] libstdc++: Avoid hard error in ranges::unique_copy [PR100770]

[Jonathan Wakely <jwakely@redhat.com>]

On 27/05/21 09:50 -0400, Patrick Palka via Libstdc++ wrote:
>On Wed, 26 May 2021, Tim Song wrote:
>
>> On Wed, May 26, 2021 at 1:43 PM Patrick Palka <ppalka@redhat.com> wrote:
>> >
>> > On Wed, 26 May 2021, Tim Song wrote:
>> > >
>> > > On Wed, May 26, 2021 at 12:00 PM Patrick Palka via Libstdc++
>> > > <libstdc++@gcc.gnu.org> wrote:
>> > > >
>> > > > -       else if constexpr (input_iterator<_Out>
>> > > > -                          && same_as<iter_value_t<_Iter>, iter_value_t<_Out>>)
>> > > > +       else if constexpr (requires { requires (input_iterator<_Out>
>> > > > +                                               && same_as<iter_value_t<_Iter>,
>> > > > +                                                          iter_value_t<_Out>>); })
>> > >
>> > > It's arguably cleaner to extract this into a concept which can then
>> > > also be used in the constraint.
>> >
>> > Sounds good, though I'm not sure what name to give to this relatively
>> > ad-hoc set of requirements.  Any suggestions? :)
>> >
>>
>> Something along the lines of "__can_reread_output", perhaps?
>
>Works for me.  Here's v2, which factors out the condition into a concept
>and defines the value_type, pointer and reference of output_iterator_wrapper
>to void but leaves alone its difference_type.  Tested on x86_64-pc-linux-gnu.

OK for 10, 11 and trunk, thanks.

>-- >8 --
>
>Subject: [PATCH] libstdc++: Avoid hard error in ranges::unique_copy [PR100770]
>
>Here, in the constexpr if condition within unique_copy, when
>input_iterator<_Out> isn't satisfied we must avoid substituting into
>iter_value_t<_Out> because the latter isn't necessarily well-formed in
>this case.  To that end, this patch factors out the condition into a
>concept and uses it throughout.
>
>This patch also makes the definition of our testsuite
>output_iterator_wrapper more minimal by defining its value_type, pointer
>and reference member types to void.  This means our existing tests for
>unique_copy already exercise the fix for this bug, so we don't need
>to add another test.  The only other fallout of this testsuite iterator
>change appears in std/ranges/range.cc, where the use of range_value_t
>on a test_output_range is now ill-formed.
>
>libstdc++-v3/ChangeLog:
>
>	* include/bits/ranges_algo.h (__detail::__can_reread_output):
>	Factor out this concept from ...
>	(__unique_copy_fn::operator()): ... here.  Use the concept
>	throughout.
>	* testsuite/std/ranges/range.cc: Remove now ill-formed use
>	of range_value_t on an output_range.
>	* testsuite/util/testsuite_iterators.h (output_iterator_wrapper):
>	Define value_type, pointer and reference member types to void.
>---
> libstdc++-v3/include/bits/ranges_algo.h          | 16 ++++++++++------
> libstdc++-v3/testsuite/std/ranges/range.cc       |  3 ---
> .../testsuite/util/testsuite_iterators.h         |  2 +-
> 3 files changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/libstdc++-v3/include/bits/ranges_algo.h b/libstdc++-v3/include/bits/ranges_algo.h
>index cda3042c11f..ecf1378742d 100644
>--- a/libstdc++-v3/include/bits/ranges_algo.h
>+++ b/libstdc++-v3/include/bits/ranges_algo.h
>@@ -1396,6 +1396,13 @@ namespace ranges
>
>   inline constexpr __unique_fn unique{};
>
>+  namespace __detail
>+  {
>+    template<typename _Out, typename _Tp>
>+      concept __can_reread_output = input_iterator<_Out>
>+	&& same_as<_Tp, iter_value_t<_Out>>;
>+  }
>+
>   template<typename _Iter, typename _Out>
>     using unique_copy_result = in_out_result<_Iter, _Out>;
>
>@@ -1407,8 +1414,7 @@ namespace ranges
> 	       projected<_Iter, _Proj>> _Comp = ranges::equal_to>
>       requires indirectly_copyable<_Iter, _Out>
> 	&& (forward_iterator<_Iter>
>-	    || (input_iterator<_Out>
>-		&& same_as<iter_value_t<_Iter>, iter_value_t<_Out>>)
>+	    || __detail::__can_reread_output<_Out, iter_value_t<_Iter>>
> 	    || indirectly_copyable_storable<_Iter, _Out>)
>       constexpr unique_copy_result<_Iter, _Out>
>       operator()(_Iter __first, _Sent __last, _Out __result,
>@@ -1432,8 +1438,7 @@ namespace ranges
> 		}
> 	    return {__next, std::move(++__result)};
> 	  }
>-	else if constexpr (input_iterator<_Out>
>-			   && same_as<iter_value_t<_Iter>, iter_value_t<_Out>>)
>+	else if constexpr (__detail::__can_reread_output<_Out, iter_value_t<_Iter>>)
> 	  {
> 	    *__result = *__first;
> 	    while (++__first != __last)
>@@ -1467,8 +1472,7 @@ namespace ranges
> 	       projected<iterator_t<_Range>, _Proj>> _Comp = ranges::equal_to>
>       requires indirectly_copyable<iterator_t<_Range>, _Out>
> 	&& (forward_iterator<iterator_t<_Range>>
>-	    || (input_iterator<_Out>
>-		&& same_as<range_value_t<_Range>, iter_value_t<_Out>>)
>+	    || __detail::__can_reread_output<_Out, range_value_t<_Range>>
> 	    || indirectly_copyable_storable<iterator_t<_Range>, _Out>)
>       constexpr unique_copy_result<borrowed_iterator_t<_Range>, _Out>
>       operator()(_Range&& __r, _Out __result,
>diff --git a/libstdc++-v3/testsuite/std/ranges/range.cc b/libstdc++-v3/testsuite/std/ranges/range.cc
>index 795aca472e5..aa29af471a4 100644
>--- a/libstdc++-v3/testsuite/std/ranges/range.cc
>+++ b/libstdc++-v3/testsuite/std/ranges/range.cc
>@@ -75,9 +75,6 @@ static_assert( same_as<std::ranges::range_difference_t<C>,
> static_assert( same_as<std::ranges::range_difference_t<O>,
> 		       std::ptrdiff_t> );
>
>-static_assert( same_as<std::ranges::range_value_t<O>,
>-		       char> );
>-
> static_assert( same_as<std::ranges::range_reference_t<I>,
> 		       char&> );
> static_assert( same_as<std::ranges::range_reference_t<O>,
>diff --git a/libstdc++-v3/testsuite/util/testsuite_iterators.h b/libstdc++-v3/testsuite/util/testsuite_iterators.h
>index 67da06b8482..4e668d648f2 100644
>--- a/libstdc++-v3/testsuite/util/testsuite_iterators.h
>+++ b/libstdc++-v3/testsuite/util/testsuite_iterators.h
>@@ -122,7 +122,7 @@ namespace __gnu_test
>    */
>   template<class T>
>   struct output_iterator_wrapper
>-  : public std::iterator<std::output_iterator_tag, T, std::ptrdiff_t, T*, T&>
>+  : public std::iterator<std::output_iterator_tag, void, std::ptrdiff_t, void, void>
>   {
>   protected:
>     output_iterator_wrapper() : ptr(0), SharedInfo(0)
>-- 
>2.32.0.rc0
>