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

[Louis Dionne via Phabricator via libcxx-commits]

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

A few more comments, but this is looking pretty good.



================
Comment at: libcxx/include/functional:3061
+                           is_move_constructible_v<_Fn>>>
+inline auto not_fn(_Fn&& __f)
+{
----------------
`inline` not needed.

This should also be `constexpr`, but I guess we should leave fixing that to whenever we implement the paper that marked it as `constexpr`.


================
Comment at: libcxx/include/functional:3085
+                                       >::value>>
+inline constexpr auto bind_front(_Fn&& __f, _Args&&... __args)
+{
----------------
`inline` is not needed here, it's a template.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:13
+
+// template <class F, class... Args> unspecified bind_front(F&&, Args&&...);
+
----------------
This should be marked as `constexpr`.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:128
+
+struct not_move_construcable
+{
----------------
Looks like a typo to me.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:234
+
+    static_assert(constexpr_test());
+
----------------
Could you instead write all the tests in a constexpr function, and call it once from `main()`, and once inside a `static_assert`? This way, we would get the full coverage for both non-constexpr and constexpr code. Like I've done in the tests of https://reviews.llvm.org/D68364.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:197
+    {
+        static_assert(!std::is_invocable<decltype(takes_not_move_constructable),
+                                         not_move_construcable>::value);
----------------
zoecarver wrote:
> Any ideas for types that are move constructible and aren't "self constructible" (i.e. `!is_constructible_v<decay_t<T>, T>`)? 
You mean a type that satisfies: `is_constructible_v<decay_t<T>, T&&> && !is_constructible_v<decay_t<T>, T>`? No, I can't think of anything.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.verify.cpp:13
+
+// template <class F, class... Args> unspecified bind_front(F&&, Args&&...);
+
----------------
`constexpr`


================
Comment at: libcxx/test/support/callable_types.h:9
+
+#ifndef CALLABLE_TYPES_H
+#define CALLABLE_TYPES_H
----------------
`TEST_CALLABLE_TYPES_H`


================
Comment at: libcxx/www/cxx2a_status.html:111
 	<tr><td><a href="https://wg21.link/P0318R1">P0318R1</a></td><td>LWG</td><td>unwrap_ref_decay and unwrap_reference</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
-	<tr><td><a href="https://wg21.link/P0356R5">P0356R5</a></td><td>LWG</td><td>Simplified partial function application</td><td>San Diego</td><td><i> </i></td><td></td></tr>
+	<tr><td><a href="https://wg21.link/P0356R5">P0356R5</a></td><td>LWG</td><td>Simplified partial function application</td><td>San Diego</td><td>Complete</td><td></td></tr>
 	<tr><td><a href="https://wg21.link/P0357R3">P0357R3</a></td><td>LWG</td><td>reference_wrapper for incomplete types</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
----------------
Please mark it complete as of 12.0. Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60368