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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__functional/bind_back.h:27
+
+#if _LIBCPP_STD_VER > 17
+
----------------
Quuxplusone wrote:
> 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.)
I would agree because of the "dropping `#if`s faster" story, but I think the benefit of using standard facilities (i.e. `invoke` instead of `__invoke`) outweighs that. WDYT?


================
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
+
----------------
Quuxplusone wrote:
> 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.
There isn't a huge benefit in doing this, since if Barry's paper gets in, `bind_back` would be enabled in C++20 (or 23) only. I'd basically rename it from `__bind_back` to `bind_back` and expose it publicly.


================
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>
+
----------------
Quuxplusone wrote:
> 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. :)
Sold, let's mechanically include everything. I will still include `__functional/compose.h` in the `compose.pass.cpp` test, since there's no plan to make it part of `<functional>` in the spec. I'll still include it in `<functional>` just for consistency, but I think it's better to include the minimal header if we're testing a pure implementation detail.


================
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 {}; }
----------------
Quuxplusone wrote:
> 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. 
Yes, it is intentional. I think I should actually put it in a test that is specific to `__perfect_forward`, but I'm on the fence because I don't want to have the tests in three places once `bind_back` makes it to the standard (`bind_front`, `bind_back` and `__perfect_forward`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107785