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
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? |
libcxx/include/functional | ||
---|---|---|
2989 | Good point. I should probably make this a different patch. | |
2994โ2995 | I don't think so. This one corresponds to line 2977. Because of how the diff is rendered, it looks like I added this one instead of the one on 3095. |
@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. |