[libcxx-commits] [PATCH] D107785: [libc++] Add the __bind_back and __compose helpers

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

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

Marking "request changes" so it shows up correctly in queues; but there's nothing big or urgent among my comments. Just consider enabling in C++14 mode, and (almost certainly IMHO) include the new files in <functional>.



================
Comment at: libcxx/include/__functional/bind_back.h:27
+
+#if _LIBCPP_STD_VER > 17
+
----------------
Could this be `> 11`?
Could it be `> 11` if you used `_VSTD::__invoke` instead of `_VSTD::invoke`?
(Besides just feeling nicer, keeping our dependencies minimal might one day help us clean up old code faster. As soon as we drop support for C++11, the whole `#if` goes away. Whereas if you make it depend on C++20, then we have to keep the `#if` in place for another 9 years until we drop support for C++17.)


================
Comment at: libcxx/include/__functional/compose.h:25
+
+#if _LIBCPP_STD_VER > 17
+
----------------
Same here: could this be `> 11`?


================
Comment at: libcxx/test/libcxx/utilities/function.objects/func.bind.partial/bind_back.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
If you do enable `__bind_back` back to C++14, then this line will need to change and you'll need to use more `::value` and less `_v` in this test.


================
Comment at: libcxx/test/libcxx/utilities/function.objects/func.bind.partial/bind_back.pass.cpp:20
+
+#include <__functional/bind_back.h> // TODO: Remove this if it gets added to <functional>
+
----------------
What's the ruling on whether `<foo>` should include `<__foo/internal_detail.h>`? I know when we were originally debating the micro-headers, the idea was floated (but not necessarily agreed upon or even remarked upon) that `<foo>` should eventually be a simple mechanical listing of all `<__foo/*.h>`, i.e., if any `<__foo/internal_detail.h>` were missing from `<foo>`, we'd call that a bug.
This also applies to D107584: Should `<concepts>` include `<__concepts/boolean_testable.h>`? Should it include `<__concepts/class_or_enum.h>`? Right now I've answered "yes" to both, following what seems like precedent:

* `<algorithm>` includes `<__algorithm/comp_ref_type.h>` and `<__algorithm/half_positive.h>`;
* `<memory>` includes `<__memory/compressed_pair.h>`;
* `<iterator>` includes `<__iterator/wrap_iter.h>`.

If this is enough precedent for you, I think we should adopt it as official (unwritten) policy and this TODO should just get DONE. :)


================
Comment at: libcxx/test/libcxx/utilities/function.objects/func.bind.partial/bind_back.pass.cpp:326-333
+      using RetT = decltype(std::__bind_back(std::move(value), 1));
+
+      static_assert( std::is_move_constructible<RetT>::value);
+      static_assert(!std::is_copy_constructible<RetT>::value);
+      static_assert(!std::is_move_assignable<RetT>::value);
+      static_assert(!std::is_copy_assignable<RetT>::value);
+
----------------
Nit: You could flip these around:
```
auto ret = std::__bind_back(std::move(value), 1);
using RetT = decltype(ret);
static_assert( std::is_move_constructible<RetT>::value);  // etc.
```


================
Comment at: libcxx/test/libcxx/utilities/function.objects/func.bind.partial/bind_back.pass.cpp:384
+
+  // Make sure __bind_back is SFINAE friendly
+  {
----------------
Nit: Lines 386–390 are about whether the unspecified type's `operator()` is SFINAE-friendly.
Lines 394–406 are about whether the `std::__bind_back` function itself is SFINAE-friendly.
Tests for both are needed and good, of course; the comment is just a little icky to conflate them.


================
Comment at: libcxx/test/libcxx/utilities/function.objects/func.bind.partial/compose.pass.cpp:61-71
+  // Make sure we can call a function that's a pointer to a member function.
+  {
+    struct MemberFunction1 {
+      constexpr Elem<0> foo() { return {}; }
+    };
+    struct MemberFunction2 {
+      constexpr MemberFunction1 bar() { return {}; }
----------------
Maybe worth testing some ref-qualified member functions too?
I notice we're not testing any of the perfect-forwarding-call-wrapper stuff here; e.g. we're never checking whether `std::move(c)` is invocable; nor whether it's //not// invocable when `F2` is not move-invocable; etc. etc. That lack may be intentional, given that we have older tests for perfect-forwarding-call-wrapper stuff elsewhere in the test suite. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107785