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

Thu Aug 18 03:00:41 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:46
+    }
+    return __len1 <=> __len2;
+  } else {
----------------
avogelsgesang wrote:
> mumbleskates wrote:
> > philnik wrote:
> > > 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. 
> > i'm not even 100% sure what is allowed to be assumed about `difference_type`; i would definitely avoid using `size_t` here and stick entirely to the iterator's difference type.
> Using `difference_type` now, and also only using this optimization if both iterator types have the same, integral `difference_type`
`difference_type` is guaranteed to be a signed integral (http://eel.is/c++draft/iterator.traits#2). Otherwise your program if ill-formed. I'd just use `ptrdiff_t` as the type here. No need to not optimize just because the `difference_type`s are different.

================
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:
> > huixie90 wrote:
> > > avogelsgesang wrote:
> > > > philnik wrote:
> > > > > avogelsgesang wrote:
> > > > > > mumbleskates 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>);
> > > > > > > > ```
> > > > > > > i'm sorry yes, `static_assert`. my bad :)
> > > > > > 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.
> >  it has these two sneaky words Equivalent to, which has a lot of very subtle implications
> 
> I am not sure how to interpret that paragraph, afaict it does not directly talk about SFINAE.
> 
> Did both Microsoft (https://github.com/microsoft/STL/blob/ebbc9908ad24dc5dbbdbce820432e79f166ec547/stl/inc/xutility#L4982) and libstdc++ (https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/libstdc%2B%2B-v3/include/bits/stl_algobase.h#L1866) get this wrong?
Ah no, Ok. I misremembered. It's only when it part of a `Constraints:` clause that you have to bring over the SFINAE fun. As I said, sneaky words :P.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395