[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>,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395