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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked an inline comment as done.
ldionne added inline comments.


================
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;
+    }
----------------
Quuxplusone wrote:
> 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.
Hmm, okay, I guess I am swayed. I'll repurpose this patch again to only remove the `-Wsign-conversion` warnings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106372