[libcxx-commits] [PATCH] D80902: [libcxx] adds lexicographical_compare_three_way

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/include/compare:644
   constexpr _CCC _Cat = sizeof...(_Ts) == 0 ? _StrongOrd
-      : __compute_comp_type(__type_kinds);
+      : __compute_comp_type<sizeof...(_Ts)>(__type_kinds);
   if constexpr (_Cat == _None)
----------------
Nit: you could avoid passing the size explicitly if you passed the array by reference.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:82
+
+  test_less<Iter1, Iter2>(a, b);
+  test_equal<Iter1, Iter2>(a, a);
----------------
That's already better, even if personally I would create a different array in each of the test_x functions.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:89
+int main(int, char**) {
+  test<input_iterator<const int*>, input_iterator<const int*> >();
+  test<input_iterator<const int*>, forward_iterator<const int*> >();
----------------
You can move all those calls to `test<Iterator, Iterator>` to another function and just call it twice from main (once in static_assert).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:24
+
+TEST_CONSTEXPR bool test_constexpr() {
+    int ia[] = {1, 2, 3};
----------------
cjdb wrote:
> curdeius wrote:
> > Constexpr test is... minimal to say the least. I think it would be better to have a single constexpr function (for applicable cases) and call it once on runtime and once on compile time.
> I can do that. This test is (mostly) a carbon copy of the test for std::lexicographical_compare, so many of your comments are also applicable to that test as well. That doesn't excuse me from improving this test, so I've applied the changes here.
> 
> Does libc++ have a way to assert based on whether it's compile-time or run-time yet?
I'm not sure if I understand your question correctly. There is `(__libcpp_)is_constant_evaluated`, but there is, to my knowledge, nothing that asserts that a path is taken on runtime/compile time.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp:12
+// template<InputIterator Iter1, InputIterator Iter2, CopyConstructible Compare>
+//   constexpr bool
+//   lexicographical_compare_three_way(Iter1 first1, Iter1 last1,
----------------
cjdb wrote:
> curdeius wrote:
> > Bool?
> Fixed. How many of your comments from the above are also applicable here?
Probably all.


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

https://reviews.llvm.org/D80902