[Bug libstdc++/101587] New: uninitialized_copy/move incorrectly uses std::min

[Bug libstdc++/101587] New: uninitialized_copy/move incorrectly uses std::min

[hewillk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

            Bug ID: 101587
           Summary: uninitialized_copy/move incorrectly uses std::min
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hewillk at gmail dot com
  Target Milestone: ---

ranges_uninitialized.h#L269:

    if constexpr (sized_sentinel_for<_ISent, _Iter>
                      && sized_sentinel_for<_OSent, _Out>
                      && is_trivial_v<_OutType>
                      && is_nothrow_assignable_v<_OutType&,
                                                 iter_reference_t<_Iter>>)
      {
        auto __d1 = __ilast - __ifirst;
        auto __d2 = __olast - __ofirst;
        return ranges::copy_n(std::move(__ifirst), std::min(__d1, __d2),
                  __ofirst);
      }

We should make sure that __d1 and __d2 are the same types before calling
std::min, the same goes for uninitialized_copy_n/move_n.

#include <memory>
#include <ranges>

int main() {
  auto r = std::views::iota(0l, 5l);
  std::array<long, 5> o;
  std::ranges::uninitialized_copy(r, o);
}

https://godbolt.org/z/fj7hM8qdx

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-07-23

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Maybe we should add this somewhere and just stop using std::min for integers:

  struct __min_fn
  {
    template<typename _Tp, typename _Up>
      typename common_type<_Tp, _Up>::type
      operator()(_Tp __t, _Up __u) const noexcept
      { return std::min<typename common_type<_Tp, _Up>::type>(__t, __u); }
  };
  _GLIBCXX17_INLINE constexpr __min_fn __min{};

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[hewillk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

--- Comment #2 from 康桓瑋 <hewillk at gmail dot com> ---
(In reply to Jonathan Wakely from comment #1)
> Maybe we should add this somewhere and just stop using std::min for integers:
> 
>   struct __min_fn
>   {
>     template<typename _Tp, typename _Up>
>       typename common_type<_Tp, _Up>::type
>       operator()(_Tp __t, _Up __u) const noexcept
>       { return std::min<typename common_type<_Tp, _Up>::type>(__t, __u); }
>   };
>   _GLIBCXX17_INLINE constexpr __min_fn __min{};

It may cause problems when comparing integers with different signedness.

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
In theory yes, but it's unlikely to be a problem in practice because the
library doesn't need to compare unsigned types with negative values. If it does
need to do that, it is either already doing so carefully and correctly, or this
helper wouldn't make things worse.

I'm not suggesting designing a general purpose replacement for std::min that
handles all the possible corner cases, I'm just suggesting a convenience
function for our own internal needs.

All the uses in <bits/ranges_uninitialized.h> are with iterator difference
types, which had better be signed, and distance(first, last) had better be
non-negative too.


include/bits/hashtable_policy.h does:

      const auto __max_width = std::min<size_t>(sizeof(size_t), 8);

This is mixing unsigned and signed, but both positive.


include/bits/locale_conv.h:

       auto __nn = std::min<streamsize>(this->epptr() - this->pptr(),
                                        __n - __done);

This compares ptrdiff_t with streamsize, which are both signed (and both values
here are guaranteed to be non-negative by construction).

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[hewillk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

--- Comment #4 from 康桓瑋 <hewillk at gmail dot com> ---
(In reply to Jonathan Wakely from comment #3)
>
> I'm not suggesting designing a general purpose replacement for std::min that
> handles all the possible corner cases, I'm just suggesting a convenience
> function for our own internal needs.

Yep, this is reasonable, just like std::__memcmp does, and it is obviously
simpler than something like std::min<std::conditional_t<(sizeof(__d1) >
sizeof(__d1)), decltype(__d1), decltype(__d2)>>(__d1, __d2), and thank you for
your detailed explanation BTW.

[Bug libstdc++/101587] ranges::uninitialized_copy/move incorrectly uses std::min

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101587

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, much simpler than that! :-)

There are lots of uses of std::min and std::max with explicit template argument
lists in the Parallel Mode headers, but I have no intention of touching them,
and they need to work as C++98 so couldn't use the function above anyway.