[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:46
+    }
+    return __len1 <=> __len2;
+  } else {
----------------
JohelEGP wrote:
> huixie90 wrote:
> > 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?
> It should if the difference type is an integer-class type.
I don't think it has any observable effects, but we shouldn't cast to different types, since that can prohibit some optimizations. 


================
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
----------------
avogelsgesang wrote:
> philnik wrote:
> > 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.
> done, except for `int*`. `int*` does not work, because the currently used `array`s are const.
> Is it fine not to test `int*`? Which additional test coverage would `int*` give us over `const int*`?
You can just take the arrays by value instead of const reference. `int*` checks that the algorithms doesn't fail for mutable types. It's not expected that it will happen, but it's also a very common scenario and quite easy to add tests for.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:62
+}
+
+constexpr bool test() {
----------------
huixie90 wrote:
> 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.
You are right that it doesn't literally say that it should be SFINAEd away, but it has these two sneaky words `Equivalent to`, which has a lot of very subtle implications. See http://eel.is/c++draft/description#structure.specifications-4 for details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395