Implementes P0356R5. Adds bind_front to functional.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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. | |
Comment Actions
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? | |
| include/functional | ||
|---|---|---|
| 3015 | By "qualify tuple" I meant the _VSTD:: isn't needed | |
Comment Actions
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.
| 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). | |
Comment Actions
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. | |
Comment Actions
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. The type we store is different from the types we're passed in. | |
| 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. | |
| 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. | |
Comment Actions
@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. | |
Comment Actions
Update based on review:
- fix decay type
- fix move return type
- universal reference parameter args
- tests