Page MenuHomePhabricator

[libcxx] adds lexicographical_compare_three_way
Needs ReviewPublic

Authored by cjdb on May 31 2020, 3:23 PM.

Details

Reviewers
EricWF
mclow.lists
ldionne
jfb
BRevzin
jdoerfert
Group Reviewers
Restricted Project
Summary

[libcxx] adds lexicographical_compare_three_way

  • adds lexicographical_compare_three_way algorithms
  • adds tests
  • replaces std::array with raw array to eliminate circular dep :-(

Diff Detail

Event Timeline

cjdb created this revision.May 31 2020, 3:23 PM
miscco added a subscriber: miscco.May 31 2020, 11:35 PM
miscco added inline comments.
libcxx/include/algorithm
5631

The identation here is off. Could you please add braces so that it is clear where the loop ends

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp
41

These are rather subtle tests. To make it imediately clear what you are actually testing I would appreciate if you could go the extra mile and add comments

// lexicographical_compare_three_way(a, b), range b shorter than range a

// lexicographical_compare_three_way(a, b), range a shorter than range b

cjdb updated this revision to Diff 267644.Jun 1 2020, 9:09 AM
cjdb marked 2 inline comments as done.
cjdb added inline comments.
libcxx/include/algorithm
5631

clang-format doesn't seem to care about this file, so I've had to do it manually. Weird.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp
41

Done, but you might ask for further improvements.

curdeius added inline comments.
libcxx/include/algorithm
619

Return type in synopsis doesn't match the one in implementation.

624

Same here.

5631

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.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp
25

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.

31

Format?

39

Can't you use std::array?

50

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.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
13

Bool?

cjdb updated this revision to Diff 268027.Jun 2 2020, 6:05 PM
cjdb marked 2 inline comments as done.

Applies some of @curdeius's suggestions.

cjdb updated this revision to Diff 268028.Jun 2 2020, 6:07 PM
cjdb marked 7 inline comments as done and an inline comment as not done.
cjdb added inline comments.
libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp
25

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?

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
13

Fixed. How many of your comments from the above are also applicable here?

cjdb added a reviewer: mpark.Jun 2 2020, 6:09 PM
cjdb removed a reviewer: mpark.
curdeius added inline comments.Jun 3 2020, 4:50 AM
libcxx/include/compare
645

Nit: you could avoid passing the size explicitly if you passed the array by reference.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp
25

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.

83

That's already better, even if personally I would create a different array in each of the test_x functions.

90

You can move all those calls to test<Iterator, Iterator> to another function and just call it twice from main (once in static_assert).

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
13

Probably all.

How do we implement the "Mandates" requirement specified here?

libcxx/include/algorithm
662

Please unconditionally include headers.

5624

blank line after after the #ifdef.

We should also gate this on the standard dialect, because three way comparison can be enabled as an extension.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp
26

You don't need to use TEST_CONSTEXPR because the test is unsupported in C++03 (where constexpr can't be spelled without Clang getting mad).

cjdb updated this revision to Diff 270596.Jun 13 2020, 6:50 PM
cjdb marked 11 inline comments as done.
cjdb marked 2 inline comments as done.
cjdb added inline comments.
libcxx/include/algorithm
5624

I think I've done this one?

libcxx/include/compare
645

I did that at first, but apparently sizeof...(_Ts) can be 0.

cjdb updated this revision to Diff 270597.Jun 13 2020, 7:20 PM
cjdb added a reviewer: Restricted Project.Jun 22 2020, 9:17 AM

As a general remark, it would be call if you added a link to standard draft or proposal (or both) in the summary of each revision. It would make reviewing a bit easier :).

Otherwise, it seems good to me. But you need to wait for libc++ reviewers.