[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/algorithm:5633
+    for (; __first1 != __last1 && __first2 != __last2; void(++__first1), void(++__first2))
+    if (auto __cmp = __comp(*__first1, *__first2); __cmp != 0)
+        return __cmp;
----------------
cjdb wrote:
> miscco wrote:
> > The identation here is off. Could you please add braces so that it is clear where the loop ends
> clang-format doesn't seem to care about this file, so I've had to do it manually. Weird.
FYI. If you use git-clang-format, it filters modified files based on their extension, and by default extensionless files are skipped. But there is a parameter for that.


================
Comment at: libcxx/include/algorithm:619
+template <class InputIterator1, class InputIterator2>
+    constexpr bool
+    lexicographical_compare_three_way(InputIterator1 first1, InputIterator1 last1,
----------------
Return type in synopsis doesn't match the one in implementation.


================
Comment at: libcxx/include/algorithm:624
+template <class InputIterator1, class InputIterator2, class Compare>
+    constexpr bool
+    lexicographical_compare_three_way(InputIterator1 first1, InputIterator1 last1,
----------------
Same here.


================
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};
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:30
+        && std::lexicographical_compare_three_way(std::begin(ib), std::end(ib), std::begin(ia), std::end(ia)) == std::strong_ordering::greater
+           ;
+}
----------------
Format?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:38
+    int ia[] = {1, 2, 3, 4};
+    const unsigned sa = sizeof(ia)/sizeof(ia[0]);
+    int ib[] = {1, 2, 3};
----------------
Can't you use `std::array`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:49
+    assert(std::lexicographical_compare_three_way(Iter1(ia),   Iter1(ia+sa), Iter2(ib+1), Iter2(ib+3)) == std::strong_ordering::less);
+    assert(std::lexicographical_compare_three_way(Iter1(ib+1), Iter1(ib+3),  Iter2(ia),   Iter2(ia+sa)) == std::strong_ordering::greater);
+}
----------------
Personally, I find it very difficult to read and to verify that you covered all possible cases of less/greater, shorter/longer, equal, empty with this style. I'd rather split them.


================
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,
----------------
Bool?


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

https://reviews.llvm.org/D80902