Add bind_front function (P0356R5)

[zoecarver]

Summary

Implementes P0356R5. Adds bind_front to functional.

[ldionne]

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

[zoecarver]

• 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.

[zoecarver]

• 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.

[zoecarver]

• Add more test cases.

[zoecarver]

@ldionne no rush, but this is ready for re-review whenever you have time.

[zoecarver]

@ldionne ping.

[curdeius]

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?

[curdeius]

It would be great if you rebased this patch to trigger current CI.

[zoecarver]

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]

• Use new status format
• Rebase on master

[curdeius]

@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

Good point. I should probably make this a different patch.

[ldionne]

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?

[zoecarver]

• Feature test macros
• Update to version 13
• Remove constexpr test
• Rebase off main
• Update tests as needed

[zoecarver]

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:

1. Wait to land this until we implement constexpr assignment operators for std::tuple.
2. Add the above check (is_constant_evaluated) to this test and remove it once constexpr assignment operators for std::tuple are implemented.
3. Just remove this part of the test entirely because it's not required by the standard.
4. Update std::tuple to have constexpr assignment operators (without properly implementing the paper).

[zoecarver]

• Add comments and remove unneeded assignment operators.

[Quuxplusone]

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).
Also I'd recommend some tests to see whether the capture is by value or by reference, e.g.

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.
Alternatively, provide and test the assignment operator but move the test into test/libcxx/utilities/ instead??

[ldionne]

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).

[zoecarver]

libcxx/include/functional

3087

Mandates: conjunction_v<is_constructible<FD, F>, is_move_constructible<FD>, is_constructible<BoundArgs, Args>..., is_move_constructible<BoundArgs>...> shall be true.

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.

[zoecarver]

• Fix based on review

[Quuxplusone]

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.

[zoecarver]

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>>.

[zoecarver]

• Properly decay args before checking is_move_constructible.

[zoecarver]

• Format bind_front test file.

[zoecarver]

• Rebase off main (somehow my last commit got added to this patch).

[zoecarver]

• Call the test function test_invocability from the main test function.

(Sorry for all the updates/emails.)

[Quuxplusone]

LGTM; just some naming nits.

libcxx/include/functional

3002

Nit: Could you call this tuple<_Bound...> __bounds or tuple<_Bound...> __tup instead?
I was really puzzled by line 3046's

__bound(_VSTD::forward<_BoundArgs>(__bound)...) { }

for a minute.
Also nit, qualifying _VSTD:: shouldn't be necessary on tuple because it's a type, not a function.

libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp

233

Utter nit: invocable, not invokable ;)
but also less nitty, isn't this just testing more like is_bind_frontable? I mean you're testing that bind_front(LocalArgs...) is makable, but you're not testing that the resulting bind object is actually invocable at all, right?

libcxx/test/support/callable_types.h

187

Nit: no newline at end of file

[ldionne]

LGTM. Apply Arthur's comments and let's ship this!

[zoecarver]

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.

[zoecarver]

• Fix review comments