[libcxx-commits] [PATCH] D60368: Add bind_front function (P0356R5)

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: include/functional:2875
+template<class _Fn, class... _Args>
+inline auto bind_front(_Fn&& __f, _Args&&... __args)
+{
----------------
zoecarver wrote:
> 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?
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.


================
Comment at: include/functional:2809
+    auto operator()(_Args&&... __args) &&
+    noexcept(noexcept(_Op::__call(_VSTD::move(_VSTD::get<_Idxs>(__bound))..., _VSTD::forward<_Args>(__args)...)))
+    -> decltype(      _Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound))..., _VSTD::forward<_Args>(__args)...))
----------------
Why is this different than below?


================
Comment at: include/functional:2857
+{
+    static_assert(sizeof...(_Args) > 0, "At least one argument must be provided");
+    return __perfect_forward<__bind_front_op, _Fn, _Args...>
----------------
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.


================
Comment at: include/functional:2858
+    static_assert(sizeof...(_Args) > 0, "At least one argument must be provided");
+    return __perfect_forward<__bind_front_op, _Fn, _Args...>
+        (_VSTD::forward<_Fn>(__f), _VSTD::forward<_Args>(__args)...);
----------------
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&`.

 


================
Comment at: test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp:126
         auto ret2 = std::move(ret);
-        assert(ret() == false);
+        assert(ret() == true);
         assert(ret2() == true);
----------------
This doesn't seem right.

It's probably related to the bug mentioned below.


================
Comment at: test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp:432
-        try {
-            (void)std::not_fn(cp);
-            assert(false);
----------------
zoecarver wrote:
> I am not sure why this was expected to throw. I //think// this can simply be moved (but I might be wrong).
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.




================
Comment at: test/support/callable_types.h:1
+#include "type_id.h"
+
----------------
Missing the license header.



================
Comment at: test/support/callable_types.h:2
+#include "type_id.h"
+
+///////////////////////////////////////////////////////////////////////////////
----------------
Missing header guards.


================
Comment at: test/support/callable_types.h:7
+
+bool returns_true() { return true; }
+
----------------
This needs `inline` now that it's in a header.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60368/new/

https://reviews.llvm.org/D60368