This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [P1065] Constexpr invoke, reference_wrapper, mem_fn, not_fn.
ClosedPublic

Authored by Quuxplusone on Dec 25 2020, 1:10 PM.

Details

Summary

I believe this is a complete implementation of P1065R2 "Constexpr INVOKE" (Tomasz Kamiński, Barry Revzin), which was adopted for C++20.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1065r2.html

This could use more constexpr tests for std::reference_wrapper<T>, but the existing tests are extremely constexpr-unfriendly and so I don't want to get into that rabbit-hole today.

I plan to push the refactoring of ForwardingCallObject as a separate preliminary commit:

[libc++] Constexpr-proof some machinery in not_fn.pass.cpp. NFCI.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 25 2020, 1:10 PM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 25 2020, 1:10 PM

Complete the implementation of __cpp_lib_constexpr_functional and set that feature macro. This involves constexprifying std::default_searcher from P1032 "Misc constexpr bits". There's more "misc" stuff in P1032, which I have not attempted to do in this PR because it doesn't affect the __cpp_lib_constexpr_functional feature macro.

https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#library-feature-test-macros

zoecarver accepted this revision.Dec 26 2020, 7:32 PM
zoecarver added a subscriber: zoecarver.

This LGTM. Thanks. Looking forward to using this in my bind_front patch ;)

libcxx/test/std/utilities/function.objects/func.invoke/invoke_constexpr.pass.cpp
269

I assume it didn't make sense to merge these with the existing tests because of functions like foo which would only work in one mode or the other?

Is there any test coverage here that isn't in the non-constexpr version? If so, would it make sense to run these tests outside of a constexpr context as well (it very well might not)?

libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp
548

This is just because std::string isn't constexpr?

Quuxplusone marked an inline comment as done.Dec 26 2020, 8:37 PM
Quuxplusone added inline comments.
libcxx/test/std/utilities/function.objects/func.invoke/invoke_constexpr.pass.cpp
269

A mix of that reason, and the reason that I wanted to make sure constexprifying things didn't somehow break the non-constexpr codepath. One thing we haven't been worrying about much with these patches, and which I suddenly decided to worry about for this patch (why? I don't know) is that when we mark all the helper/component functions like foo with TEST_CONSTEXPR_CXX20, we are losing test coverage of how the thing works with non-constexpr functions. For invoke, that suddenly seemed important to test.

There is no test coverage here that isn't in the non-constexpr version. I suppose it wouldn't hurt to run these tests also outside of constexpr; I'll add that.

libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp
548

Correct. The obj it's calling is actually callable with auto..., so it doesn't matter what types we pass to it. I don't know the original author's motivation for using std::string here; probably just it seemed convenient at the time.

I considered removing all this file's usage of <string>, but it seemed like an unnecessary rabbit hole at the moment.

Quuxplusone marked an inline comment as done.

Run constexpr tests also in non-constexpr mode. Rebase on main.

Poke the buildbot. (It's been stuck in "pre-merge checks" for 10 hours.)

Fix a failing test (don't put constexpr on non-const methods in C++11).
If buildkite is green after this, imma push it.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 28 2020, 10:25 AM
This revision was automatically updated to reflect the committed changes.