Page MenuHomePhabricator

[Libcxx] Finish implementing Paper 0202R3
ClosedPublic

Authored by ldionne on Jul 21 2020, 4:29 PM.

Details

Reviewers
Nicholas-Baron
ldionne
Group Reviewers
Restricted Project
Commits
rGb552a30283ce: [libc++] Finish implementing P0202R3
Summary

cppreference lists the support for this paper as partial.
I found 4 functions which the paper marks as constexpr, but did not use the appropriate macro.

Diff Detail

Event Timeline

Nicholas-Baron created this revision.Jul 21 2020, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 4:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I see no tests.

I apologize, as this is my first time contributing. I do not know where the tests for the other functions that were made constexpr are.
My testing plan would at least involve showing that the functions compile in a constexpr context in C++20 and not in earlier versions.

miscco added a subscriber: miscco.Jul 21 2020, 11:28 PM

Thanks a lot for taking the time.

Unfortunately putting _LIBCPP_CONSTEXPR_AFTER_CXX17 in front of the function is only half way through the implementation. As you can see for example in move_backward the implementation falls back to _VSTD::__rotate which is also defined in <algorithm>.
Now to use move_backward in a constexpr context you need make all subsequent functions such as __rotate constexpr. Sometimes this is easy, sometimes it is just a rabbit hole ;)

Regarding the tests I would like to point you to the libcxx/test/std/algorithms subfolder. For example you can find the tests for move_backward here: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp

As you can see there is a convenient test() function. What you have to do is add TEST_CONSTEXPR_CXX20 before it and then in addition to calling it at runtime also call it inside a constexpr context. The easiest way to do so is to simply change the function return type to bool and add a return true at the end. Then you only need to duplicate all the calls to test via static_assert(test());

Note that you would also need to guard these C++20 additions via #if TEST_STD_VER >= 20. I appologize that the macros are a mess each using its own selection of after foo, >=foo and foo.

[...]

Regarding the tests I would like to point you to the libcxx/test/std/algorithms subfolder. For example you can find the tests for move_backward here: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp

As you can see there is a convenient test() function. What you have to do is add TEST_CONSTEXPR_CXX20 before it and then in addition to calling it at runtime also call it inside a constexpr co

@Nicholas-Baron Indeed, thanks for working on your first contribution! What @miscco said above is great, and I would add that you probably want to go and add the tests first, then run the test suite and notice that the expected tests are failing. Then you can go ahead and add the required annotations to the library. One example of a patch that constexpr-ified a bunch of tests is https://reviews.llvm.org/D80452, you can take a look at that one.

Also, this appears to have some overlap with https://reviews.llvm.org/D65721. Perhaps you can coordinate with @zoecarver and pick up the work in D65721?

[...]
Also, this appears to have some overlap with https://reviews.llvm.org/D65721. Perhaps you can coordinate with @zoecarver and pick up the work in D65721?

Thank you @Idionne for pointing me to that patch. This will help focus my efforts.
The move and move_backward functions are covered in that diff, so I will remove my changes to those functions.
However, merge and rotate_copy both are marked by the paper as constexpr, but are not addressed in the mentioned patch (and so I will address them here).

Removed parts covered in https://reviews.llvm.org/D65721 and uncommented a test.

Passed tests and made one internal function constexpr

The builds are failing with this line standing out
ld.lld: error: unable to find library -lcxxabi_shared

Hopefully, I can land D65721 sometime this week, and then you can rebase this patch on master. Otherwise, feel free to take anything you need from that patch.

libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate_copy.pass.cpp
31

Rather than returning the tested condition, the convention is to simply assert that the result is what you expect and return true from the function. If the assertion fires the function will not be able to be evaluated as a constant expression and the test will fail.

Applied constexpr testing conventions suggested by @zoecarver.

zoecarver added inline comments.Jul 23 2020, 12:37 PM
libcxx/test/std/algorithms/alg.sorting/alg.merge/merge.pass.cpp
36

Almost perfect. Can you make each check an individual assert? That makes it easier to figure out what went wrong if the tests start failing.

This should be:

assert(std::distance(...) == (...));
assert(*it == 0);
assert(std::equal(...));
Nicholas-Baron marked an inline comment as done.

Decomposed asserts in tests

zoecarver added inline comments.Jul 23 2020, 1:11 PM
libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate_copy.pass.cpp
40

Do you think you could make this test a constexpr and return true at the end? That way we test the same thing in both constexpr and non-constexpr mode. TEST_CONSTEXPR should work in all modes.

Nicholas-Baron marked an inline comment as done.

Used TEST_CONSTEXPR on the merge and rotate_copy tests.

Nicholas-Baron added inline comments.Jul 23 2020, 3:47 PM
libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate_copy.pass.cpp
40

Would this mean changing the call site as well to utilize the returned true or is ignoring the return value acceptable in this case?

ldionne commandeered this revision.Sep 14 2020, 2:00 PM
ldionne added a reviewer: Nicholas-Baron.

Commandeering to show exactly what we meant by the requested test changes, however we're very very close here. Thanks a lot for your contribution @Nicholas-Baron !

ldionne updated this revision to Diff 291679.Sep 14 2020, 2:00 PM

Finish up the tests

ldionne accepted this revision.Sep 14 2020, 2:02 PM
ldionne added inline comments.
libcxx/test/std/algorithms/alg.sorting/alg.merge/merge.pass.cpp
49

It would have been lovely to make all these tests constexpr as well, but they are using <random>. We could rework the tests to make more of it constexpr friendly if desired, but I think this is good enough as is for now.

This revision is now accepted and ready to land.Sep 14 2020, 2:02 PM

For the record, I tested this in all standard modes using

for std in c++11 c++14 c++17 c++2a; do ninja -C build check-cxx-deps && ./build/bin/llvm-lit -sv libcxx/test/std/algorithms --param std=$std; done
This revision was landed with ongoing or failed builds.Sep 14 2020, 2:03 PM
This revision was automatically updated to reflect the committed changes.