[PATCH] libstdc++: Replace use of __mindist in ranges::uninitialized_xxx algos [PR101587]
Jonathan Wakely
jwakely@redhat.com
Fri Apr 4 10:27:41 GMT 2025
On Fri, 4 Apr 2025 at 11:05, Hewill Kang <hewillk@gmail.com> wrote:
>
> Linstdc++’s __max_size_type is only explicitly converted to other integral if I recall correctly.
Hmm, yes, you're right. So why don't we get errors for __mindist not
converting back to an integer?
Aha, it's because the only view that uses __max_diff_type is
iota_view, and that's an input range not an output range, so you can
only copy *from* iota_view, not to it. And that means for:
ranges::uninitialized_copy(from, to)
only the from range uses __max_diff_type, and so we convert
distance(to) to __max_diff_type, and it works fine.
We would only need an explicit conversion if distance(to) was an
integer-like class type, and distance(from) was not.
So we need a test with a custom output range that uses
iter_different_t<decltype(iota(__int128(0), __int128(0)))>.
>
> Jonathan Wakely <jwakely@redhat.com>於 2025年4月4日 週五，17:56寫道：
>>
>> On Fri, 4 Apr 2025 at 10:32, Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>> >
>> >
>> >
>> > On Fri, Apr 4, 2025 at 11:10 AM Jonathan Wakely <jwakely@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
>> >>
>>
More information about the Libstdc++
mailing list