On Fri, 4 Apr 2025 at 10:32, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Fri, Apr 4, 2025 at 11:10 AM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> In r15-8980-gf4b6acfc36fb1f I introduced a new function object for
>> finding the smaller of two distances. In bugzilla Hewill Kang pointed
>> out that we still need to explicitly convert the result back to the
>> right difference type, because the result might be an integer-like class
>> type that doesn't convert to an integral type explicitly.
>>
>> Rather than doing that conversion in the __mindist function object, I
>> think it's simpler to remove it again and just do a comparison and
>> assignment. We always want the result to have a specific type, so we can
>> just check if the value of the other type is smaller, and then convert
>> that to the other type if so.
>>
>> libstdc++-v3/ChangeLog:
>>
>> PR libstdc++/101587
>> * include/bits/ranges_uninitialized.h (__detail::__mindist):
>> Remove.
>> (ranges::uninitialized_copy, ranges::uninitialized_copy_n)
>> (ranges::uninitialized_move, ranges::uninitialized_move_n): Use
>> comparison and assignment instead of __mindist.
>> ---
>>
>> Tested x86_64-linux.
>
> The revert makes sense, but this integer-like-classes are causing a lot of
> churm.
> I would like to see some tests for this scenario in algorithms tests.
> Using iota<__int128>(0, 5) is good way to produce integer-like difference
> type,
> that is sized. (iota<__int128>(0) | views::take(5)) creates not-sized one.
Yes, but our integer-like class type (__max_size_type) is implicitly
convertible to integer types, so is more user-friendly than required
by the standard. I don't think we want to change that, but it means
it's easy to miss places where portable code should really be using an
explicit conversion.
Actually I'm not sure this patch is even needed ... because the only
integer-like class type that is allowed is the one provided by the
library ... and we don't need the explicit conversion for our type.
But I like the simplification anyway.
Anyway, I'll add some more tests using iota_view, but they were
already passing before this patch, that's why I didn't bother.
> With test LGTM.
>
>>
>>
>> .../include/bits/ranges_uninitialized.h | 46 ++++++-------------
>> 1 file changed, 14 insertions(+), 32 deletions(-)
>>
>> diff --git a/libstdc++-v3/include/bits/ranges_uninitialized.h
>> b/libstdc++-v3/include/bits/ranges_uninitialized.h
>> index b5580073a6a..12a714b68aa 100644
>> --- a/libstdc++-v3/include/bits/ranges_uninitialized.h
>> +++ b/libstdc++-v3/include/bits/ranges_uninitialized.h
>> @@ -263,26 +263,6 @@ namespace ranges
>> inline constexpr __uninitialized_value_construct_n_fn
>> uninitialized_value_construct_n;
>>
>> - namespace __detail
>> - {
>> - // This is only intended for finding smaller iterator differences below,
>> - // not as a general purpose replacement for std::min.
>> - struct __mindist_fn
>> - {
>> - template<typename _Dp1, typename _Dp2>
>> - constexpr common_type_t<_Dp1, _Dp2>
>> - operator()(_Dp1 __d1, _Dp2 __d2) const noexcept
>> - {
>> - // Every C++20 iterator I satisfies weakly_incrementable<I> which
>> - // requires signed-integer-like<iter_difference_t<I>>.
>> - static_assert(std::__detail::__is_signed_integer_like<_Dp1>);
>> - static_assert(std::__detail::__is_signed_integer_like<_Dp2>);
>> - return std::min<common_type_t<_Dp1, _Dp2>>(__d1, __d2);
>> - }
>> - };
>> - inline constexpr __mindist_fn __mindist{};
>> - }
>> -
>> template<typename _Iter, typename _Out>
>> using uninitialized_copy_result = in_out_result<_Iter, _Out>;
>>
>> @@ -305,10 +285,10 @@ namespace ranges
>> && is_trivially_assignable_v<_OutType&,
>> iter_reference_t<_Iter>>)
>> {
>> - auto __d1 = __ilast - __ifirst;
>> - auto __d2 = __olast - __ofirst;
>> - return ranges::copy_n(std::move(__ifirst),
>> - __detail::__mindist(__d1, __d2), __ofirst);
>> + auto __d = __ilast - __ifirst;
>> + if (auto __d2 = __olast - __ofirst; __d2 < __d)
>> + __d = static_cast<iter_difference_t<_Iter>>(__d2);
>> + return ranges::copy_n(std::move(__ifirst), __d, __ofirst);
>> }
>> else
>> {
>> @@ -356,9 +336,9 @@ namespace ranges
>> && is_trivially_assignable_v<_OutType&,
>> iter_reference_t<_Iter>>)
>> {
>> - auto __d = __olast - __ofirst;
>> - return ranges::copy_n(std::move(__ifirst),
>> - __detail::__mindist(__n, __d), __ofirst);
>> + if (auto __d = __olast - __ofirst; __d < __n)
>> + __n = static_cast<iter_difference_t<_Iter>>(__d);
>> + return ranges::copy_n(std::move(__ifirst), __n, __ofirst);
>> }
>> else
>> {
>> @@ -397,11 +377,12 @@ namespace ranges
>> && is_trivially_assignable_v<_OutType&,
>>
>> iter_rvalue_reference_t<_Iter>>)
>> {
>> - auto __d1 = __ilast - __ifirst;
>> - auto __d2 = __olast - __ofirst;
>> + auto __d = __ilast - __ifirst;
>> + if (auto __d2 = __olast - __ofirst; __d2 < __d)
>> + __d = static_cast<iter_difference_t<_Iter>>(__d2);
>> auto [__in, __out]
>> = ranges::copy_n(std::make_move_iterator(std::move(__ifirst)),
>> - __detail::__mindist(__d1, __d2), __ofirst);
>> + __d, __ofirst);
>> return {std::move(__in).base(), __out};
>> }
>> else
>> @@ -452,10 +433,11 @@ namespace ranges
>> && is_trivially_assignable_v<_OutType&,
>>
>> iter_rvalue_reference_t<_Iter>>)
>> {
>> - auto __d = __olast - __ofirst;
>> + if (auto __d = __olast - __ofirst; __d < __n)
>> + __n = static_cast<iter_difference_t<_Iter>>(__d);
>> auto [__in, __out]
>> = ranges::copy_n(std::make_move_iterator(std::move(__ifirst)),
>> - __detail::__mindist(__n, __d), __ofirst);
>> + __n, __ofirst);
>> return {std::move(__in).base(), __out};
>> }
>> else
>> --
>> 2.49.0
>>
