[libc++] Avoid triggering warnings for implicit conversion

[ldionne]

Summary

This started as fixing a typo in a ADDITIONAL_COMPILE_FLAGS directive
which turned out to uncover a few places where we warned about signedness
changes.

As a fly-by fix, this updates the various __advance overloads
for style consistency.

[ldionne]

Repurpose the patch -- the main purpose is now fixing std::advance

[• Quuxplusone]

libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp

68–72

I don't see anything in https://eel.is/c++draft/iterator.operations that forbids vendors from doing a conversion to iter_difference_t here. (In fact, both std::next and std::ranges::advance require that conversion, so it shouldn't be surprising if std::advance also does it.)
The Standard says only: "Increments i by n if n is non-negative, and decrements i by -n otherwise." Presumably the intention is to use the iterator's operator+= to do that (although the Standard doesn't say); and the iterator is not guaranteed to provide any satisfactory operator+= except the one taking iter_difference_t. So if you're trying to pass Distance unmodified to operator+=, you're not always going to be able to satisfy the Standard's behavioral requirement here. The only way to guarantee that you're getting a valid, concept-semantic-modeling overload of operator+= is to do the conversion first.

This is https://cplusplus.github.io/LWG/issue3439 ; see also https://cplusplus.github.io/LWG/issue3213 which mandates convertible-to-integral for Size.

TLDR: I think this patch is misguided. If anything, we should be testing for

template<class Distance>
struct DistanceChecker {
    ~~~
    TEST_CONSTEXPR_CXX17 DistanceChecker& operator+=(difference_type) { ~~~ }
    TEST_CONSTEXPR_CXX17 DistanceChecker& operator+=(int) = delete;
}
std::advance(it, 1);  // should advance by difference_type(1), not int(1)

However, in practice I would recommend not testing for that, either; that kind of iterator type seems to violate common sense re the proper design of overload sets.

[ldionne]

libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp

68–72

Hmm, okay, I guess I am swayed. I'll repurpose this patch again to only remove the -Wsign-conversion warnings.

[ldionne]

Repurpose.

[• Quuxplusone]

LGTM at this point if buildkite is happy (which it seems it's not, yet).
If you think the advance stuff needs more thought/discussion, then feel free to ship the hash and tuple pieces, which seem utterly non-controversial, just to get them out of the way.

libcxx/include/__iterator/advance.h

64–65

_IntegralDistance is used only once. If you want, you could roll these back up into a single parameter

class = _EnableIf<is_integral<decltype(_VSTD::__convert_to_integral(declval<_Distance>()))>::value>

[ldionne]

libcxx/include/__iterator/advance.h

64–65

I think I'll leave it as-is, I find it easier to read.

[ldionne]

Fix CI (hopefully).

[• Quuxplusone]

@ldionne: Serendipitously, @jwakely just alerted me to LWG's Tentatively Ready on @tcanens' paper P2393R0 "Cleaning up integer-class types", which specifically mandates (the effect of) an explicit cast from decltype(n) to iter_difference_t<I> every time the library clauses talk about i + n or i - n. See the added section marked '?', in green.
The state-of-trunk after D106372 seems pretty darn close to what's about to be mandated — and in particular, D106372 didn't make things worse. :) Good!

libcxx/include/__functional/hash.h

if (__len > 8) {
       return __hash_len_16(__a, __rotate_by_at_least_1(__b + __len, __len)) ^ __b;
     }
     if (__len >= 4) {
       const uint32_t __a = __loadword<uint32_t>(__s);
       const uint32_t __b = __loadword<uint32_t>(__s + __len - 4);
       return __hash_len_16(__len + (__a << 3), __b);
     }
     if (__len > 0) {
-      const unsigned char __a = __s[0];
-      const unsigned char __b = __s[__len >> 1];
-      const unsigned char __c = __s[__len - 1];
+      const unsigned char __a = static_cast<unsigned char>(__s[0]);
+      const unsigned char __b = static_cast<unsigned char>(__s[__len >> 1]);
+      const unsigned char __c = static_cast<unsigned char>(__s[__len - 1]);
       const uint32_t __y = static_cast<uint32_t>(__a) +
                            (static_cast<uint32_t>(__b) << 8);
       const uint32_t __z = __len + (static_cast<uint32_t>(__c) << 2);
       return __shift_mix(__y * __k2 ^ __z * __k3) * __k2;
     }
     return __k2;
   }
 

libcxx/include/__iterator/advance.h

 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-template <class _InputIter>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 void
-__advance(_InputIter& __i, typename iterator_traits<_InputIter>::difference_type __n, input_iterator_tag) {
+template <class _InputIter, class _Distance>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+void __advance(_InputIter& __i, _Distance __n, input_iterator_tag) {
   for (; __n > 0; --__n)
     ++__i;
 }
 
-template <class _BiDirIter>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 void
-__advance(_BiDirIter& __i, typename iterator_traits<_BiDirIter>::difference_type __n, bidirectional_iterator_tag) {
+template <class _BiDirIter, class _Distance>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+void __advance(_BiDirIter& __i, _Distance __n, bidirectional_iterator_tag) {
   if (__n >= 0)
     for (; __n > 0; --__n)
       ++__i;
   else
     for (; __n < 0; ++__n)
       --__i;
 }
 
-template <class _RandIter>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 void
-__advance(_RandIter& __i, typename iterator_traits<_RandIter>::difference_type __n, random_access_iterator_tag) {
+template <class _RandIter, class _Distance>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+void __advance(_RandIter& __i, _Distance __n, random_access_iterator_tag) {
   __i += __n;
 }
 
 template <
     class _InputIter, class _Distance,
-    class = typename enable_if<is_integral<decltype(_VSTD::__convert_to_integral(declval<_Distance>()))>::value>::type>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 void advance(_InputIter& __i, _Distance __orig_n) {
-  typedef decltype(_VSTD::__convert_to_integral(__orig_n)) _IntegralSize;
+    class _IntegralSize = decltype(_VSTD::__convert_to_integral(declval<_Distance>())),
+    class = _EnableIf<is_integral<_IntegralSize>::value> >
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+void advance(_InputIter& __i, _Distance __orig_n) {
   _IntegralSize __n = __orig_n;
   _LIBCPP_ASSERT(__n >= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value,
                  "Attempt to advance(it, n) with negative n on a non-bidirectional iterator");
   _VSTD::__advance(__i, __n, typename iterator_traits<_InputIter>::iterator_category());
 }
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 namespace ranges {
 // [range.iter.op.advance]

libcxx/include/tuple

get(const tuple<_Tp...>&& __t) _NOEXCEPT
     return static_cast<const type&&>(
              static_cast<const __tuple_leaf<_Ip, type>&&>(__t.__base_).get());
 }
 
 #if _LIBCPP_STD_VER > 11
 
 namespace __find_detail {
 
-static constexpr size_t __not_found = -1;
+static constexpr size_t __not_found = static_cast<size_t>(-1);
 static constexpr size_t __ambiguous = __not_found - 1;
 
 inline _LIBCPP_INLINE_VISIBILITY
 constexpr size_t __find_idx_return(size_t __curr_i, size_t __res, bool __matches) {
     return !__matches ? __res :
         (__res == __not_found ? __curr_i : __ambiguous);
 }
 

libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp

 //
 // template <BidirectionalIterator Iter, class Distance>
 //   constexpr void advance(Iter& i, Distance n);
 //
 // template <RandomAccessIterator Iter, class Distance>
 //   constexpr void advance(Iter& i, Distance n);
 
 // Make sure we catch forced conversions to the difference_type if they happen.
-// ADDITIONAL_COMPILER_FLAGS: -Wsign-conversion
+// ADDITIONAL_COMPILE_FLAGS: -Wsign-conversion
 
 #include <iterator>
 #include <cassert>
 #include <cstddef>
 #include <type_traits>
 
 #include "test_macros.h"
 #include "test_iterators.h"
 
 template <class Distance, class It>
 TEST_CONSTEXPR_CXX17
 void check_advance(It it, Distance n, It result)
 {
     static_assert(std::is_same<decltype(std::advance(it, n)), void>::value, "");
     std::advance(it, n);
     assert(it == result);
 }
 
+// Dummy random access iterator that only checks that the type passed to its operator+=
+// is the given Distance type.
+template<class Distance>
+struct DistanceChecker {
+    typedef std::random_access_iterator_tag iterator_category;
+    typedef int                             value_type;
+    typedef std::ptrdiff_t                  difference_type;
+    typedef int*                            pointer;
+    typedef int&                            reference;
+
+    TEST_CONSTEXPR reference operator*() const;
+    TEST_CONSTEXPR pointer operator->() const;
+
+    TEST_CONSTEXPR DistanceChecker& operator++();
+    TEST_CONSTEXPR DistanceChecker operator++(int);
+
+    TEST_CONSTEXPR DistanceChecker& operator--();
+    TEST_CONSTEXPR DistanceChecker operator--(int);
+
+    friend TEST_CONSTEXPR DistanceChecker operator+(difference_type, DistanceChecker);
+    TEST_CONSTEXPR DistanceChecker operator+(difference_type) const;
+    TEST_CONSTEXPR DistanceChecker& operator-=(difference_type);
+    TEST_CONSTEXPR DistanceChecker operator-(difference_type n) const;
+
+    TEST_CONSTEXPR reference operator[](difference_type) const;
+
+    template<class T>
+    TEST_CONSTEXPR_CXX17 DistanceChecker& operator+=(T) {
+        static_assert(std::is_same<T, Distance>::value, "Was a conversion performed inside std::advance?");
+        return *this;
+    }
+};
+
 TEST_CONSTEXPR_CXX17 bool tests()
 {
     const char* s = "1234567890";
+
+    // Check with iterator_traits::difference_type
+    {
         typedef std::iterator_traits<const char*>::difference_type Distance;
         check_advance<Distance>(cpp17_input_iterator<const char*>(s), 10, cpp17_input_iterator<const char*>(s+10));
         check_advance<Distance>(forward_iterator<const char*>(s), 10, forward_iterator<const char*>(s+10));
         check_advance<Distance>(bidirectional_iterator<const char*>(s+5), 5, bidirectional_iterator<const char*>(s+10));
         check_advance<Distance>(bidirectional_iterator<const char*>(s+5), -5, bidirectional_iterator<const char*>(s));
         check_advance<Distance>(random_access_iterator<const char*>(s+5), 5, random_access_iterator<const char*>(s+10));
         check_advance<Distance>(random_access_iterator<const char*>(s+5), -5, random_access_iterator<const char*>(s));
         check_advance<Distance>(s+5, 5, s+10);
         check_advance<Distance>(s+5, -5, s);
+    }
 
     // Also check with other distance types
+    {
+        check_advance<int>(cpp17_input_iterator<const char*>(s), 10, cpp17_input_iterator<const char*>(s+10));
+        check_advance<int>(forward_iterator<const char*>(s), 10, forward_iterator<const char*>(s+10));
+        check_advance<int>(bidirectional_iterator<const char*>(s), 10, bidirectional_iterator<const char*>(s+10));
+        check_advance<int>(random_access_iterator<const char*>(s), 10, random_access_iterator<const char*>(s+10));
+    }
+
+    // Check with unsigned distance types to catch signedness-change issues
+    {
         check_advance<std::size_t>(cpp17_input_iterator<const char*>(s), 10u, cpp17_input_iterator<const char*>(s+10));
         check_advance<std::size_t>(forward_iterator<const char*>(s), 10u, forward_iterator<const char*>(s+10));
         check_advance<std::size_t>(bidirectional_iterator<const char*>(s), 10u, bidirectional_iterator<const char*>(s+10));
-    check_advance<std::size_t>(random_access_iterator<const char*>(s), 10u, random_access_iterator<const char*>(s+10));
+        // Note: We don't check random_access_iterator with an unsigned distance, because a random_access_iterator is only
+        // required to have operator+=(difference_type), which is signed and makes advance(it, unsigned) complain about
+        // signedness change when calling operator+=.
+    }
+
+    // Make sure we pass the distance through to operator+= as-is.
+    {
+        typedef std::size_t Dist;
+        typedef DistanceChecker<Dist> It;
+        static_assert(!std::is_same<Dist, It::difference_type>::value, ""); // test the test
+        It it;
+        std::advance(it, (Dist)0);
+    }
 
     return true;
 }
 
 int main(int, char**)
 {
     tests();
 #if TEST_STD_VER >= 17
     static_assert(tests(), "");
 #endif
     return 0;
 }