Implementes P0356R5. Adds bind_front to functional.
Details
- Reviewers
mclow.lists EricWF howard.hinnant ldionne • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG84a50f5911bf: [libc++] Add bind_front function (P0356R5).
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I don't believe this patch implements the perfect forwarding call wrapper requirements laid out in the paper. Take a look at the not_fn implementation as an example of this (perhaps even generalize it's implementation to be used here as well).
include/functional | ||
---|---|---|
2980 ↗ | (On Diff #194430) | Types don't need to be qualified. Only functions calls that potentially use ADL do. |
2992 ↗ | (On Diff #194430) | No need to quality tuple. |
After looking at not_fn, I think I can simplify this a lot.
include/functional | ||
---|---|---|
2992 ↗ | (On Diff #194430) | Is there a better way to store the bound args? |
include/functional | ||
---|---|---|
2992 ↗ | (On Diff #194430) | By "qualify tuple" I meant the _VSTD:: isn't needed |
I updated this patch to use a more generalized perfect forwarding call struct. I modeled it after not_fn and got a lot of help from @EricWF. I also updated not_fn to use this more generalized struct.
I added tests similar to not_fn's tests to ensure perfect forwarding functions properly. I also generalized the structs used in those tests so I would not have to duplicate code. I think these tests cover the perfect forwarding implementation, but I can add more if needed.
include/functional | ||
---|---|---|
2875 ↗ | (On Diff #196282) | The return type of this function is not specified, it might be beneficial to make it a constexpr (though that would require other parts of this to be updated and might not work). Thoughts? |
test/std/utilities/function.objects/func.bind_front/bind_front.fail.cpp | ||
30 ↗ | (On Diff #196282) | I am unsure of how to format the fail tests. |
test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp | ||
432 ↗ | (On Diff #196282) | I am not sure why this was expected to throw. I think this can simply be moved (but I might be wrong). |
More to come.
include/functional | ||
---|---|---|
2852 ↗ | (On Diff #196377) | Formatting nit. Note how the other bits of code have three identical chunks of code, all aligned so that you could see that they were identical. Please do that here. See the code that you're replacing (lines 2815->17) for an example. |
test/std/utilities/function.objects/func.bind_front/bind_front.fail.cpp | ||
33 ↗ | (On Diff #196377) | This line can be removed; if the compilation never succeeds. |
36 ↗ | (On Diff #196377) | This one too |
test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
56 ↗ | (On Diff #196377) | Formatting nit. When ganging a bunch of static asserts, leave a space after the '(', so that the negations are easily visible. See test/std/utilities/meta/meta.unary/meta.unary.cat/nullptr.pass.cpp as an example. |
There are a couple of bugs in this patch related to the value-categories we should (A) store, and (B) initialize the storage with.
The specification specifies both cases here: http://eel.is/c++draft/func.bind.front#1
Let me know if I can help interpret that.
include/functional | ||
---|---|---|
2875 ↗ | (On Diff #196282) | The standard library is not allowed to add constexpr as an extension. Also the return type has nothing to do with the constexpr-ness of the function. |
2809 ↗ | (On Diff #196993) | Why is this different than below? |
2857 ↗ | (On Diff #196993) | Why does at least one argument need to be provided? Sure, it seems silly to std::bind_front no arguments, but I don't see anything in the standard forbidding it. |
2858 ↗ | (On Diff #196993) | This doesn't correctly implement the value categories for the functor nor arguments. The type we store is different from the types we're passed in. |
test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp | ||
126 ↗ | (On Diff #196993) | This doesn't seem right. It's probably related to the bug mentioned below. |
432 ↗ | (On Diff #196282) | It's expected to throw because the functor must be copied or moved into the not_fn wrapper. ThrowsOnCopy doesn't have a move constructor so its copy constructor is always used. The reason this test isn't failing anymore is because of a bug in your implementation. |
test/support/callable_types.h | ||
1 ↗ | (On Diff #196993) | Missing the license header. |
2 ↗ | (On Diff #196993) | Missing header guards. |
7 ↗ | (On Diff #196993) | This needs inline now that it's in a header. |
@EricWF, Thanks for the review :) I will try to apply those changes today.
include/functional | ||
---|---|---|
2857 ↗ | (On Diff #196993) | For some reason, I thought I remembered reading something about that in the paper, but looking back at it I cannot find it. I will remove this. |
2858 ↗ | (On Diff #196993) | I seem to have skipped over that part, my bad. Will fix. |
Update based on review:
- fix decay type
- fix move return type
- universal reference parameter args
- tests
- updates not_fn constraints
- adds constexpr
- updates perfect forwarding to be perfect
- enables constraints on bind_front and perfect forwarding wrapper
- more tests for bind_front
- fix 3184
- implement P1651
I'm not seeing any issues with the perfect forwarding anymore. This looks pretty good, with a few comments.
libcxx/include/functional | ||
---|---|---|
3010 | Can you please throw in _LIBCPP_INLINE_VISIBILITY (and below)? | |
3051 | If you used fold a expression like (is_move_constructible<_Bound>::value && ...), I believe you could get rid of __all_true completely. | |
3084 | I think you need to check all the requirements at this point (from the paper): conjunction_v<is_constructible<FD, F>, is_move_constructible<FD>, is_constructible<BoundArgs, Args>..., is_move_constructible<BoundArgs>...> As it stands, you seem to be testing for is_constructible<FD, F> && is_move_constructible<FD> here, but you're delaying the checks for is_constructible<BoundArgs, Args> and is_move_constructible<BoundArgs> until the constructor of __perfect_forward_impl. I think you need to check everything eagerly here, or some SFINAE-able errors may become hard errors instead. Furthermore, I would suggest going the easy way and sticking to the way the spec words it: you could just use conjunction instead of fold expressions or other clever tricks above -- this will also have the correct short-circuit behavior for evaluating the various is_constructible<BoundArgs, Args> and is_move_constructible<BoundArgs, Args> traits. | |
3220 | Can you please drop these unrelated whitespace changes? | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.fail.cpp | ||
1 ↗ | (On Diff #216761) | You could rename this to .verify.cpp instead since it's using clang-verify. |
libcxx/include/functional | ||
---|---|---|
3051 | To be clear, I don't think you'll need that at all anymore if you implement my suggestion of using conjunction_v below. |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
---|---|---|
198 | Any ideas for types that are move constructible and aren't "self constructible" (i.e. !is_constructible_v<decay_t<T>, T>)? |
A few more comments, but this is looking pretty good.
libcxx/include/functional | ||
---|---|---|
3089 | inline is not needed here, it's a template. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
129 | Looks like a typo to me. | |
235 | Could you instead write all the tests in a constexpr function, and call it once from main(), and once inside a static_assert? This way, we would get the full coverage for both non-constexpr and constexpr code. Like I've done in the tests of https://reviews.llvm.org/D68364. | |
libcxx/test/support/callable_types.h | ||
10 | TEST_CALLABLE_TYPES_H | |
libcxx/www/cxx2a_status.html | ||
111 ↗ | (On Diff #292851) | Please mark it complete as of 12.0. Same below. |
libcxx/include/functional | ||
---|---|---|
3066 | inline not needed. This should also be constexpr, but I guess we should leave fixing that to whenever we implement the paper that marked it as constexpr. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
14 | This should be marked as constexpr. | |
198 | You mean a type that satisfies: is_constructible_v<decay_t<T>, T&&> && !is_constructible_v<decay_t<T>, T>? No, I can't think of anything. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.verify.cpp | ||
14 | constexpr |
- Make all constructors and members in callable_types constexpr.
- Make invoke use __invoke_constexpr after C++17.
- Update bind_front.pass.cpp to run all tests in a constexpr context as well.
- Remove inline prefix.
- constexpr -> _LIBCPP_CONSTEXPR_AFTER_CXX11 in front of invoke
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
---|---|---|
198 | OK. Do we even need the second check, then? It seems odd the paper would have it if it's redundant. But maybe it was just an oversight. | |
235 | Good idea. Got it working but had to update invoke to be constexpr after C++17. |
libcxx/include/functional | ||
---|---|---|
2989 | I guess you should add tests for constexpr to https://github.com/llvm/llvm-project/blob/master/libcxx/test/std/utilities/function.objects/func.invoke/invoke.pass.cpp. | |
2994–2995 | Spurious #endif? |
@zoecarver, for constexpr invoke, the paper of interest is http://wg21.link/P01065. It concerns some other functions as well, notably not_fn.
libcxx/include/functional | ||
---|---|---|
2989 |
|
I think this looks good, but it needs to be rebased on top of main. invoke has been made constexpr by @Quuxplusone since then.
If CI passes, LGTM. Consider fixing my comment about static_assert, but it's non-blocking. You should also update the release target (it's not 12 anymore, but 13).
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
---|---|---|
237 | It's weird to call this inside static_assert, since we're already calling the whole test() inside static_assert. Is this needed at all now? |
- Feature test macros
- Update to version 13
- Remove constexpr test
- Rebase off main
- Update tests as needed
libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp | ||
---|---|---|
145–146 | @ldionne I did have one problem after rebasing. Because this is implemented with std::tuple, the assignment operator isn't constexpr. I see a few options here:
|
libcxx/include/functional | ||
---|---|---|
3067 | Remove space between > ( | |
3087 | is_move_constructible<decay_t<_Args>>... perhaps? It looks like _Args can contain lvalue-reference types, and also lvalue-reference-to-array types. If this is really a bug I found, please add a regression test too. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
38 | These basic tests don't include any tests for rvalues, e.g. bind_front(add, m, 1). auto a = std::bind_front(add, m, 1); m = 42; assert(a() == 2); | |
109 | I think the comment is wrong: nc(x) would call operator() const &, not operator() const &&, because nc would be an lvalue at that point. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.verify.cpp | ||
50 | Would be nice to fix all the "No newline at end of file" warnings. | |
libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp | ||
145–146 | FWIW, I vote for #3 — std::not_fn(x) isn't supposed to be assignable. It might even be user-friendly to delete its assignment operator so that libc++ users don't start relying on it and then break when they move to a different STL vendor. |
I'll mark this as "requires changes" so it shows up properly in the queue.
libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp | ||
---|---|---|
145–146 | Yeah, I agree with Arthur about doing (3). |
libcxx/include/functional | ||
---|---|---|
3087 |
Additionally, the bound args are decayed meaning that all the args must be move or copy constructible. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
109 | I love trying to figure out what past me was thinking when I wrote this test :P Anyway, I think I got this from section 3.5 of the paper, so you're right, this should be const & not const &&. I'd like to think this was just a typo, but the name no_const_rvalue seems particularly incriminating in the case that I actually thought it was the latter. Anyway, I'll fix it. | |
libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp | ||
145–146 | OK. Want me to delete the assignment operators? I don't care one way or another, but it might break some people who are (unfortunately) relying on it. |
libcxx/include/functional | ||
---|---|---|
3087 | Ah, I think I get it (or at least I see something I'd missed the first time around). int a[10]; int f(int *); auto fa = std::bind_front(f, a); works by decaying a into an int* for storage in the closure object. But here's an example that triggers my problem, when tested against the current patch: struct S { explicit S() { } S(S&); }; S s; void f(S&); auto ff = std::bind_front(f, s); // OK auto gg = std::move(ff); // cascading errors deep inside the template stuff Here s is an S&, so _Args is S&, so it's true that is_move_constructible_v<_Args> even though not is_move_constructible_v<decay_t<_Args>>. I think either std::bind_front(f, s) should be ill-formed (my preference), or else it should produce a closure object with no move-constructor. |
libcxx/include/functional | ||
---|---|---|
3087 | I think you're right. This was just an oversight on my part. Because the standard specifically says to check is_move_constructible<decay_t<_Args>>. |
- Call the test function test_invocability from the main test function.
(Sorry for all the updates/emails.)
LGTM; just some naming nits.
libcxx/include/functional | ||
---|---|---|
3002 | Nit: Could you call this tuple<_Bound...> __bounds or tuple<_Bound...> __tup instead? __bound(_VSTD::forward<_BoundArgs>(__bound)...) { } for a minute. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
233 | Utter nit: invocable, not invokable ;) | |
libcxx/test/support/callable_types.h | ||
187 | Nit: no newline at end of file |
Thanks!
libcxx/include/functional | ||
---|---|---|
3002 | Thanks. Done and done. I updated __bound -> __bound_. | |
libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp | ||
233 | Fair enough. I was thinking about it more like, "is bind_front invocable" not "is the result invocable." But I think is_bind_frontable is more clear anyway, so I'll update it. |
I guess you should add tests for constexpr to https://github.com/llvm/llvm-project/blob/master/libcxx/test/std/utilities/function.objects/func.invoke/invoke.pass.cpp.