[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 inline comments.


================
Comment at: libcxx/include/functional:3084
+                                       is_constructible<decay_t<_Args>, _Args>...,
+                                       is_move_constructible<_Args>...
+                                       >::value>>
----------------
Quuxplusone wrote:
> `is_move_constructible<decay_t<_Args>>...` perhaps? It looks like `_Args` can contain lvalue-reference types, and also lvalue-reference-to-array types.
> 
> If this is really a bug I found, please add a regression test too.
> Mandates: conjunction_v<is_constructible<FD, F>, is_move_constructible<FD>, is_constructible<BoundArgs, Args>..., is_move_constructible<BoundArgs>...> shall be true.

Additionally, the bound args are decayed meaning that all the args must be move or copy constructible. 


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:108
+    // return [nc = no_const_rvalue{}, x] { return nc(x); };
+    // Above would not work because it would look for a () const && overload.
+    return std::bind_front(no_const_rvalue{}, x);
----------------
Quuxplusone wrote:
> I think the comment is wrong: `nc(x)` would call `operator() const &`, not `operator() const &&`, because `nc` would be an lvalue at that point.
I love trying to figure out what past me was thinking when I wrote this test :P

Anyway, I think I got this from section 3.5 of the paper, so you're right, this should be `const &` not `const &&`. I'd like to think this was just a typo, but the name `no_const_rvalue` seems particularly incriminating in the case that I actually thought it was the latter. Anyway, I'll fix it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60368