[libcxx-commits] [PATCH] D106372: [libc++] Don't perform implicit conversion in advance, take 2

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp:68-72
+    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;
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106372/new/

https://reviews.llvm.org/D106372