[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:31-34
+  if constexpr (is_base_of_v<random_access_iterator_tag,
+                             typename iterator_traits<_InputIterator1>::iterator_category> &&
+                is_base_of_v<random_access_iterator_tag,
+                             typename iterator_traits<_InputIterator2>::iterator_category>) {
----------------



================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:47
+    return __len1 <=> __len2;
+  } else {
+    // Unoptimized implementation which compares the iterators against the end in every loop iteration
----------------
It would be great if you could provide a benchmark to show the performance difference.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:75
+  // Both inputs empty
+  assert((test_lexicographical_compare<Iter1, Iter2>(array<int, 0>{}, array<int, 0>{}, std::strong_ordering::equal)));
+  // Left input empty
----------------
We normally assert inside the test function. That lets us see the checks directly, avoids forgetting an `assert()` elsewhere and removes a lot of noise.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:110-118
+  // Both iterator types are random access iterators -> triggers a special fast path inside `lexicographical_compare_three_way`
+  test_iterator_types<const int*, const int*>();
+  test_iterator_types<contiguous_iterator<const int*>, cpp20_random_access_iterator<const int*>>();
+  // One of the two iterator types is not a random access iterator -> fallback to slower algorithm
+  test_iterator_types<const int*, forward_iterator<const int*>>();
+  test_iterator_types<forward_iterator<const int*>, const int*>();
+  // Check with various other iterator types
----------------
Please check the complete cartesian product of `cpp17_input_iterator`, `forward_iterator`, `bidirectional_iterator`, `random_access_iterator`, `contiguous_iterator`, `const int*` and `int*`. Also, `cpp20_random_access_iterator` is just a C++17 input iterator.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:62
+}
+
+constexpr bool test() {
----------------
avogelsgesang wrote:
> philnik wrote:
> > avogelsgesang wrote:
> > > mumbleskates wrote:
> > > > avogelsgesang wrote:
> > > > > huixie90 wrote:
> > > > > > The spec has explicitly specifies the return type ` -> decltype(__comp(*__first1, *__first2))` and this has a SFINAE effect.
> > > > > > It would be good to test the SFINAE effect as well (if __comp is not callbale then the function should be SFINAEed out)
> > > > > Sorry, but I don't quite understand this point. How should I check for SFINAE here?
> > > > in C++20 (which you have the luxury of relying upon since this function is new in C++20) you can just make a concept and then `static_check` it.
> > > still can't follow. Did you mean `static_assert`? Or is `static_check` something different?
> > > How would I use a concept to `static_assert` that `std::lexicographical_compare_three_way` is excluded from a function overload set due to SFINAE?
> > You can do something like
> > ```
> > template <class T>
> > concept HasLexicographicalCompare = requires (T whatever) { std::lexicographical_compare_three_way(whatever); };
> > 
> > static_assert(hasLexicographicalCompare<CorrectType>)
> > static_assert(!HasLexicographicalCompare<IncorrectType>);
> > ```
> Thanks for that example! I was able to use this for `lexicographical_compare_three_way_comp.pass.cpp`.
> 
> However, I am still struggling to understand what you want to me to change in this file, i.e. `lexicographical_compare_three_way.pass.cpp` where we don't pass a comparator.
> 
> What I could come up with so far is:
> 
> ```
> template <class T>
> concept has_lexicographical_compare = requires (T t) { std::lexicographical_compare_three_way(std::declval<T*>(), std::declval<T*>(), std::declval<T*>(), std::declval<T*>()); };
> 
> // Test that `std::lexicographical_compare_three_way` accepts valid types
> static_assert(has_lexicographical_compare<int>);
> static_assert(has_lexicographical_compare<WeakInt>);
> static_assert(has_lexicographical_compare<PartialInt>);
> // Test that `std::lexicographical_compare_three_way` rejects invalid types
> static_assert(!has_lexicographical_compare<UnexpectedlyComparableInt>);
> static_assert(!has_lexicographical_compare<UncomparableInt>);
> ```
> 
> However that led to the error
> 
> ```
> In file included from /home/tsi/avogelsgesang/Documents/llvm-project/libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:18:
> In file included from /home/tsi/avogelsgesang/Documents/llvm-project/build/include/c++/v1/algorithm:1771:
> /home/tsi/avogelsgesang/Documents/llvm-project/build/include/c++/v1/__algorithm/lexicographical_compare_three_way.h:71:10: error: no matching function for call to 'lexicographical_compare_three_way'  return std::lexicographical_compare_three_way(__first1, __last1, __first2, __last2, compare_three_way());                                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
Yes, that's a bug in your code :P. You have to add `-> decltype(std::lexicographical_compare_three_way(__first1, __last1, __first2, __last2, compare_three_way()))` to the overload. Also, the `T t` isn't just there for good looks ;-). In your case you can rewrite it to
```
template <class T, class U>
concept has_lexicographical_compare = requires (T first1, T last1, U first2, U last2) { std::lexicographical_compare_three_way(first1, last1, first2, last2); };
```
and then also check with different iterator types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395