Re: [PATCH v2] libstdc++: Replace use of __mindist in ranges::uninitialized_xxx algos [PR101587]
LGTM.
Here are some valid examples that are currently rejected by libstdc++ in
gnu-mode (which would be resolved by this patch):
https://godbolt.org/z/c14T6dazc <https://godbolt.org/z/eTeEPrf8E>
#include
#include
int main() {
int x[5] = {};
auto out = std::views::iota(__int128(0), __int128(5))
| std::views::transform([&](int i) -> auto& { return x[i]; });
std::ranges::uninitialized_copy(std::views::single(0), out);
}
Jonathan Wakely 於 2025年4月4日 週五 下午6:52寫道：
> 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.
> *
> testsuite/20_util/specialized_algorithms/uninitialized_copy/constrained.cc:
> Check with ranges that use integer-like class type for
> difference type.
> *
> testsuite/20_util/specialized_algorithms/uninitialized_move/constrained.cc:
> Likewise.
>
> Reviewed-by: Tomasz Kaminski
> Reviewed-by: Hewill Kang
>
> ---
>
> Here's a v2 patch with added tests. No change to the code in the
> header, just to the tests.
>
> Tested x86_64-linux.
>
> 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
> - constexpr common_type_t<_Dp1, _Dp2>
> - operator()(_Dp1 __d1, _Dp2 __d2) const noexcept
> - {
> - // Every C++20 iterator I satisfies weakly_incrementable which
> - // requires signed-integer-like>.
> - static_assert(std::__detail::__is_signed_integer_like<_Dp1>);
> - static_assert(std::__detail::__is_signed_integer_like<_Dp2>);
> - return std::min>(__d1, __d2);
> - }
> -};
> -inline constexpr __mindist_fn __mindist{};
> - }
> -
> template
> 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>(__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>(__d);
> + return ranges::copy_n(std::move(__ifirst), __n, __ofirst);
> }
> else
> {
> @@ -397,11 +377,12 @@ namespace ranges
>
Re: [PATCH] libstdc++: Replace use of __mindist in ranges::uninitialized_xxx algos [PR101587]
Linstdc++’s __max_size_type is only explicitly converted to other integral
if I recall correctly.
Jonathan Wakely 於 2025年4月4日 週五，17:56寫道：
> On Fri, 4 Apr 2025 at 10:32, Tomasz Kaminski wrote:
> >
> >
> >
> > On Fri, Apr 4, 2025 at 11:10 AM Jonathan Wakely
> 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
> >> - constexpr common_type_t<_Dp1, _Dp2>
> >> - operator()(_Dp1 __d1, _Dp2 __d2) const noexcept
> >> - {
> >> - // Every C++20 iterator I satisfies weakly_incrementable
> which
> >> - // requires signed-integer-like>.
> >> - static_assert(std::__detail::__is_signed_integer_like<_Dp1>);
> >> - static_assert(std::__detail::__is_signed_integer_like<_Dp2>);
> >> - return std::min>(__d1, __d2);
> >> - }
> >> -};
> >> -inline constexpr __mindist_fn __mindist{};
> >> - }
> >> -
> >>template
> >> 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;
> >> -
Re: [PATCH] libstdc++: Replace use of __mindist in ranges::uninitialized_xxx algos [PR101587]
> > 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 Currently, for libstdc++, only iota_view and repeat_view use integer-class types. libc++ doesn't have integer-class types, not sure why they intended not to implement one. MSVC-STL uses this type in other places, such as the difference type of cartesian_product in some cases. It is reasonable to infer that this integer-class type will be used in libstdc++'s cartesian_product_view in the future because there is a *TODO* there, or it may be used to join_view or concat_view to resolve LWG 3666 <https://cplusplus.github.io/LWG/issue3666> or LWG 4073 <https://cplusplus.github.io/LWG/issue4073>. Jonathan Wakely 於 2025年4月4日 週五 下午6:27寫道： > On Fri, 4 Apr 2025 at 11:05, Hewill Kang 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. > > > > > > Jonathan Wakely 於 2025年4月4日 週五，17:56寫道： > >> > >> On Fri, 4 Apr 2025 at 10:32, Tomasz Kaminski > wrote: > >> > > >> > > >> > > >> > On Fri, Apr 4, 2025 at 11:10 AM Jonathan Wakely > 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 te
Re: [PATCH] libstdc++: Replace use of __mindist in ranges::uninitialized_xxx algos [PR101587]
And iota<__int128>(0) | views::take(5) | views::transform([&v](auto i) ->
auto& { return v[i]; }) to create output range particularly for this case,
I think?
Tomasz Kaminski 於 2025年4月4日 週五，17:31寫道：
>
>
> On Fri, Apr 4, 2025 at 11:10 AM Jonathan Wakely
> 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.
> 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
>> - constexpr common_type_t<_Dp1, _Dp2>
>> - operator()(_Dp1 __d1, _Dp2 __d2) const noexcept
>> - {
>> - // Every C++20 iterator I satisfies weakly_incrementable
>> which
>> - // requires signed-integer-like>.
>> - static_assert(std::__detail::__is_signed_integer_like<_Dp1>);
>> - static_assert(std::__detail::__is_signed_integer_like<_Dp2>);
>> - return std::min>(__d1, __d2);
>> - }
>> -};
>> -inline constexpr __mindist_fn __mindist{};
>> - }
>> -
>>template
>> 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>(__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>(__d);
>> +
