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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/functional:3064
+{
+    return __perfect_forward<__not_fn_op, _Fn> (_VSTD::forward<_Fn>(__f));
+}
----------------
Remove space between `> (`


================
Comment at: libcxx/include/functional:3084
+                                       is_constructible<decay_t<_Args>, _Args>...,
+                                       is_move_constructible<_Args>...
+                                       >::value>>
----------------
`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.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:37
+    auto a = std::bind_front(add, m, n);
+    assert(a() == 3);
+
----------------
These basic tests don't include any tests for rvalues, e.g. `bind_front(add, m, 1)`.
Also I'd recommend some tests to see whether the capture is by value or by reference, e.g.

    auto a = std::bind_front(add, m, 1);
    m = 42;
    assert(a() == 2);


================
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);
----------------
I think the comment is wrong: `nc(x)` would call `operator() const &`, not `operator() const &&`, because `nc` would be an lvalue at that point.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.verify.cpp:49
+    return 0;
+}
----------------
Would be nice to fix all the "No newline at end of file" warnings.


================
Comment at: libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp:149
 #if defined(_LIBCPP_VERSION)
-        ret = ret2;
-        assert(ret() == true);
-        assert(ret2() == true);
+        if (!std::is_constant_evaluated()) {
+            ret = ret2;
----------------
zoecarver wrote:
> @ldionne I did have one problem after rebasing. Because this is implemented with `std::tuple`, the assignment operator isn't constexpr. I see a few options here: 
> 
> 1. Wait to land this until we implement constexpr assignment operators for `std::tuple`.
> 2. Add the above check (`is_constant_evaluated`) to this test and remove it once constexpr assignment operators for `std::tuple` are implemented.
> 3. Just remove this part of the test entirely because it's not required by the standard. 
> 4. Update `std::tuple` to have constexpr assignment operators (without properly implementing the paper).
FWIW, I vote for #3 — `std::not_fn(x)` isn't supposed to be assignable. It might even be user-friendly to //delete// its assignment operator so that libc++ users don't start relying on it and then break when they move to a different STL vendor.
Alternatively, provide and test the assignment operator but move the test into `test/libcxx/utilities/` instead??


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60368