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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:44-45
+                  "A non-integral difference_type is undefined behavior");
+    static_assert(is_integral_v<typename iterator_traits<_InputIterator2>::difference_type>,
+                  "A non-integral difference_type is undefined behavior");
+    auto __len1    = __last1 - __first1;
----------------
Mordante wrote:
> At Discord we discovered concepts give nicer error messages. Can you test whether that's true here too, by using `integral` instead of `is_integral_v`?
> 
> Since you use two asserts, lets give additional information; alternatively merge them in one assert.
Instead of having a lot of `static_assert`s here, maybe we want to add the exposition only concepts for `Cpp17Iterator`s from the standard somewhere and `static_assert` that here?


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:48
+    auto __len2    = __last2 - __first2;
+    auto __min_len = __len1 < __len2 ? __len1 : __len2;
+    for (decltype(__min_len) __i = 0; __i < __min_len;) {
----------------
Mordante wrote:
> Why not using `std::min` here? I assume it is because the types of `__len1` and `__len2` can differ.
> 
> I'm not happy with all the `auto`s in this code; it's hard to understand what the intended types are and whether these are the correct intended types. Please remove the `auto`s so it's clear what the types are.
> 
> I really would like something along the lines of
> ```
> __iter_diff_t<_InputIterator1> __len1 = __last1 - __first1;
> __iter_diff_t<_InputIterator2> __len2 = __last2 - __first2;
> auto __min = std::min<common_type_t<decltype(__len1), decltype(__len2)>(__len1, __len2));
> ```
> This is more verbose, but at least I can understand how the types are defined and whether this is as intended.
> 
I very much disagree with you here. IMO the `__iter_diff_t<_InputIterator1>` and so on just make the code more verbose and give the option to get it wrong. `__last1 - __first1` will always return a difference type. That's not exactly surprising. Also, you want to use `decltype()` for the min for some reason. I guess because otherwise it's //really// long. How does that help with understanding the code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395