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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 2 inline comments as done.
zoecarver added a comment.

@EricWF, Thanks for the review :) I will try to apply those changes today.



================
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...>
----------------
EricWF wrote:
> 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.
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. 


================
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)...);
----------------
EricWF wrote:
> 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&`.
> 
>  
I seem to have skipped over that part, my bad. Will fix. 


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

https://reviews.llvm.org/D60368