Page MenuHomePhabricator

Add bind_front function (P0356R5)
Needs ReviewPublic

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
3003

Types don't need to be qualified. Only functions calls that potentially use ADL do.

3015

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
3015

Is there a better way to store the bound args?

EricWF added inline comments.Apr 10 2019, 9:40 AM
include/functional
3015

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
2857

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
31

I am unsure of how to format the fail tests.

test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp
432

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
2833

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
34

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

37

This one too

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

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
2809

Why is this different than below?

2857

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.

2859

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.

2860

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

This doesn't seem right.

It's probably related to the bug mentioned below.

432

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
2

Missing the license header.

3

Missing header guards.

8

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
2859

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.

2860

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