This is an archive of the discontinued LLVM Phabricator instance.

Add bind_front function (P0356R5)
ClosedPublic

Authored by zoecarver on Apr 6 2019, 1:51 PM.

Details

Summary

Implementes P0356R5. Adds bind_front to functional.

Diff Detail

Event Timeline

zoecarver created this revision.Apr 6 2019, 1:51 PM
zoecarver updated this revision to Diff 194430.Apr 9 2019, 5:54 PM

Fix return in tests and update www.

EricWF requested changes to this revision.Apr 9 2019, 9:49 PM

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.

This revision now requires changes to proceed.Apr 9 2019, 9:49 PM
zoecarver marked an inline comment as done.Apr 10 2019, 7:33 AM

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?

EricWF added inline comments.Apr 10 2019, 9:40 AM
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.

zoecarver marked 3 inline comments as done.Apr 23 2019, 10:55 AM
zoecarver added inline comments.
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).

zoecarver updated this revision to Diff 196377.Apr 23 2019, 7:42 PM

Fix spacing / formatting.

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.

zoecarver marked 4 inline comments as done.Apr 27 2019, 2:49 PM
zoecarver updated this revision to Diff 196993.Apr 27 2019, 2:51 PM

Fix styling issues

EricWF requested changes to this revision.Apr 27 2019, 10:50 PM

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.
See http://eel.is/c++draft/func.bind.front#1

The type we store is different from the types we're passed in.
The functor is decayed (std::decay) and the arguments are decayed as well, unless they're a std::reference_wrapper<T>, in which case they transform
into T&.

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.
You're not decaying the type of the functor before copying it into the tuple, so you're storing a reference to it instead.

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.

This revision now requires changes to proceed.Apr 27 2019, 10:50 PM
zoecarver marked 2 inline comments as done.Apr 28 2019, 8:49 AM

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

zoecarver marked 7 inline comments as done.May 1 2019, 5:49 PM
zoecarver updated this revision to Diff 197682.May 1 2019, 5:54 PM

Update based on review:

  • fix decay type
  • fix move return type
  • universal reference parameter args
  • tests
zoecarver updated this revision to Diff 216761.Aug 22 2019, 7:55 PM
  • 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

@EricWF any more comments on this?

ldionne requested changes to this revision.Sep 14 2020, 1:07 PM

I'm not seeing any issues with the perfect forwarding anymore. This looks pretty good, with a few comments.

libcxx/include/functional
3007

Can you please throw in _LIBCPP_INLINE_VISIBILITY (and below)?

3048

If you used fold a expression like (is_move_constructible<_Bound>::value && ...), I believe you could get rid of __all_true completely.

3081

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.

3217

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.

This revision now requires changes to proceed.Sep 14 2020, 1:07 PM
zoecarver added inline comments.Sep 15 2020, 6:01 PM
libcxx/include/functional
3048

I think I'm going to update this to use the following which will also allow me to get rid of __all_true:

std::is_move_constructible<std::tuple<_Bound...>>::value
3081

I think you're right. I'll update it.

ldionne added inline comments.Sep 16 2020, 2:57 PM
libcxx/include/functional
3048

To be clear, I don't think you'll need that at all anymore if you implement my suggestion of using conjunction_v below.

zoecarver edited the summary of this revision. (Show Details)
  • Rebase and address review comments
  • Add _LIBCPP_INLINE_VISIBILITY
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 11:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver added inline comments.Sep 18 2020, 11:12 AM
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>)?

  • Remove rejected files and files from incorrectly applied diff
ldionne requested changes to this revision.Sep 18 2020, 12:33 PM

A few more comments, but this is looking pretty good.

libcxx/include/functional
3086

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.

ldionne added inline comments.Sep 18 2020, 12:33 PM
libcxx/include/functional
3063

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

This revision now requires changes to proceed.Sep 18 2020, 12:33 PM
zoecarver updated this revision to Diff 293033.Sep 20 2020, 2:24 PM
  • 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 updated this revision to Diff 293034.Sep 20 2020, 2:26 PM
  • 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 updated this revision to Diff 293889.Sep 23 2020, 4:09 PM
  • Add more test cases.

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

curdeius added inline comments.
libcxx/include/functional
2986
2991–2992

Spurious #endif?

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

zoecarver added inline comments.Nov 29 2020, 9:26 AM
libcxx/include/functional
2986

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

2991–2992

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 updated this revision to Diff 308217.Nov 29 2020, 9:38 AM
  • Use new status format
  • Rebase on master

@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
2986

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

ldionne accepted this revision.Feb 3 2021, 2:24 PM
ldionne added a subscriber: Quuxplusone.

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 updated this revision to Diff 321272.Feb 3 2021, 5:19 PM
  • Feature test macros
  • Update to version 13
  • Remove constexpr test
  • Rebase off main
  • Update tests as needed
zoecarver added inline comments.Feb 3 2021, 5:23 PM
libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp
149–154

@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 updated this revision to Diff 321275.Feb 3 2021, 5:26 PM
  • Add comments and remove unneeded assignment operators.
libcxx/include/functional
3064

Remove space between > (

3084

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
37

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);
108

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
49

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
149–154

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 requested changes to this revision.Feb 9 2021, 8:47 AM

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
149–154

Yeah, I agree with Arthur about doing (3).

This revision now requires changes to proceed.Feb 9 2021, 8:47 AM
zoecarver marked 2 inline comments as done.Feb 9 2021, 11:21 AM
zoecarver added inline comments.
libcxx/include/functional
3084

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
108

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
149–154

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 updated this revision to Diff 322460.Feb 9 2021, 11:45 AM
  • Fix based on review
Quuxplusone added inline comments.Feb 9 2021, 1:20 PM
libcxx/include/functional
3084

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 added inline comments.Feb 15 2021, 4:46 PM
libcxx/include/functional
3084

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 updated this revision to Diff 323844.Feb 15 2021, 4:48 PM
  • Properly decay args before checking is_move_constructible.
zoecarver updated this revision to Diff 323845.Feb 15 2021, 4:49 PM
  • Format bind_front test file.
zoecarver updated this revision to Diff 323846.Feb 15 2021, 4:50 PM
  • Rebase off main (somehow my last commit got added to this patch).
zoecarver updated this revision to Diff 323847.Feb 15 2021, 4:54 PM
  • Call the test function test_invocability from the main test function.

(Sorry for all the updates/emails.)

Quuxplusone accepted this revision.Mar 1 2021, 1:52 PM

LGTM; just some naming nits.

libcxx/include/functional
2999

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 accepted this revision.Mar 2 2021, 7:34 AM

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

Thanks!

libcxx/include/functional
2999

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 updated this revision to Diff 327617.Mar 2 2021, 4:16 PM
  • Fix review comments
This revision was not accepted when it landed; it landed in state Needs Review.Mar 2 2021, 4:18 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.