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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:39
+    size_t __len2    = __last2 - __first2;
+    size_t __min_len = std::min(__len1, __len2);
+    for (size_t __i = 0; __i < __min_len; ++__i, ++__first1, ++__first2) {
----------------
You can search for the usage of the header `<__undef_macros>` to see how to undef min/max macros


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:46
+    }
+    return __len1 <=> __len2;
+  } else {
----------------
agree with @philnik that providing benchmark to show the difference.

question: the standard specifies `return (last1 - first1) <=> (last2 - first2);` but here you are using `size_t` instead of iterator's `difference_type`. so effectively
`return static_cast<size_t>(last1 - first1) <=> static_cast<size_t>(last2 - first2);`. Does this have an observable difference?


================
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:
> > > 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.
> > You have to add `-> decltype(std::lexicographical_compare_three_way(__first1, __last1, __first2, __last2, compare_three_way()))` to the overload
> 
> Afaict, this explicit return type is not present in the standard (https://eel.is/c++draft/alg.sorting#alg.three.way). Or am I looking in the wrong place?
@philnik The spec only explicitly specifies the return type for the comparator overload and not the other overload.
So only the comparator overload has SFINAE and for the other overload implementations are free to hard error in the function body.

So re the testing, we only need to test the SFINAE for comparator overload. for the none comparator overload, it is expected that the test won't work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395