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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:46
+    }
+    return __len1 <=> __len2;
+  } else {
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:62
+}
+
+constexpr bool test() {
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395