Page MenuHomePhabricator

Add bind_front function (P0356R5)
Needs ReviewPublic

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



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

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.

2992 ↗(On Diff #194430)

Is there a better way to store the bound args?

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

30 ↗(On Diff #196282)

I am unsure of how to format the fail tests.

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.

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.

33 ↗(On Diff #196377)

This line can be removed; if the compilation never succeeds.

36 ↗(On Diff #196377)

This one too

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/ 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:

Let me know if I can help interpret that.

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

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.

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.

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?