Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne added inline comments.Sep 18 2020, 12:33 PM
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

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
2989
2994–2995

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

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
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 updated this revision to Diff 321275.Feb 3 2021, 5:26 PM
  • Add comments and remove unneeded assignment operators.
Quuxplusone added inline comments.Feb 4 2021, 11:00 AM
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 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
145–146

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
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 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
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 added inline comments.Feb 15 2021, 4:46 PM
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 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
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 accepted this revision.Mar 2 2021, 7:34 AM

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

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