[libcxx-commits] [PATCH] D131395: [libc++] Implement `lexicographical_compare_three_way`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:43
+    static_assert(is_integral_v<typename iterator_traits<_InputIterator1>::difference_type>,
+                  "A non-integral difference_type is undefined behavior");
+    static_assert(is_integral_v<typename iterator_traits<_InputIterator2>::difference_type>,
----------------
philnik wrote:
> ldionne wrote:
> > var-const wrote:
> > > Nit: I think this would read a little better with a verb, e.g. `Using a non-integral difference_type is undefined behavior`.
> > > 
> > > Also, this seems a little overkill -- I'm pretty sure there are many places in algorithms where we subtract random access iterators and expect to get an integral type, without checking. I don't object to the `static_assert`s, but the comment (starting from `We rely on the fact...`) seems a little unnecessary to me (the part about undefined behavior is already captured in the static assertion).
> > @avogelsgesang @philnik @var-const 
> > 
> > Is there a reason why we're testing for this *here* specifically? Isn't this a general property that we require from iterators?
> > 
> > Also, why don't we check that the difference type is signed? That's what https://eel.is/c++draft/iterator.iterators#2.2 says.
> > 
> > FWIW, I believe it would be preferable to add this assertion in a more principled way across the library, and also (especially) to do it in a separate patch. We probably need to add a temporary escape hatch for this one cause I would assume that it may break a lot of users.
> > @avogelsgesang @philnik @var-const 
> > 
> > Is there a reason why we're testing for this *here* specifically? Isn't this a general property that we require from iterators?
> We're adding it here because it's a new algorithm, so we can't break people.
> > Also, why don't we check that the difference type is signed? That's what https://eel.is/c++draft/iterator.iterators#2.2 says.
> I don't remember why we only check that it's integral; I assume it was just an oversight.
> > FWIW, I believe it would be preferable to add this assertion in a more principled way across the library, and also (especially) to do it in a separate patch. We probably need to add a temporary escape hatch for this one cause I would assume that it may break a lot of users.
> I agree that it would make sense to add it to all the algorithms, but I don't think it makes a lot of sense to add an escape hatch to a new algorithm.
Good point about the new algorithm. Let's add it here unconditionally. And then we can pursue lifting this into something like `static_assert(__iterator_requirements_whatever<_It>)` that we can perhaps sprinkle in a few places (with a escape hatch for existing algorithms).

Basically I'd like to avoid this being just a one-off. I think this assert is a good idea and would like to expand its use.

Let's address the `signed integral` issue before we ship, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395