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.
Details
Details
- Reviewers
Nicholas-Baron ldionne - Group Reviewers
Restricted Project - Commits
- rGb552a30283ce: [libc++] Finish implementing P0202R3
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
Comment Actions
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.
Comment Actions
@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?
Comment Actions
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).
Comment Actions
The builds are failing with this line standing out
ld.lld: error: unable to find library -lcxxabi_shared
Comment Actions
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 | ||
|---|---|---|
| 30 | 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. | |
| 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(...)); | |
| libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate_copy.pass.cpp | ||
|---|---|---|
| 22–23 | 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. | |
| libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/rotate_copy.pass.cpp | ||
|---|---|---|
| 22–23 | Would this mean changing the call site as well to utilize the returned true or is ignoring the return value acceptable in this case? | |
Comment Actions
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 !
| libcxx/test/std/algorithms/alg.sorting/alg.merge/merge.pass.cpp | ||
|---|---|---|
| 46 | 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. | |
Comment Actions
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
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.