[libcxx-commits] [PATCH] D103753: [libc++] [P0619] Add _LIBCPP_ABI_NO_BINDER_BASES and remove binder typedefs in C++20.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__functional_base:58
     typedef bool __result_type;  // used by valarray
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
+    typedef bool result_type;
----------------
Mordante wrote:
> I'm not entirely happy with this construction. IIRC @ldionne considered not adding these `_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS` flags anymore. If we don't add this enable macro we can simplify the code by only omitting the base class only in C++20 with the unstable API.
Well, I'd definitely like to get the better ABI layout even in `-std=c++14` mode; there's nothing C++20-specific about removing the base classes. It's just that in C++17-and-earlier we still need to provide the binder typedefs. This is the same general shape as what we did for `reverse_iterator`: remove the `std::iterator` base class whenever possible, but then continue to provide the iterator typedefs whenever necessary.

As I said somewhere else recently, I'm strongly in favor of keeping the `_LIBCPP_ABI` and `_LIBCPP_ENABLE_CXX??_REMOVED` macros, for internal documentation: it's easy to grep for and see all the places affected by a given section of p0619, and it's easy to see which sections we've actually implemented and which we still need to do.

I do think the `_LIBCPP_SUPPRESS_DEPRECATED_PUSH`/`_LIBCPP_SUPPRESS_DEPRECATED_POP` construction is confusing and noisy (and error-prone, as I found out by initially misplacing the push outside the `#if` on `bit_not` ;)) — but I'm trusting @ldionne that it's required.


================
Comment at: libcxx/test/std/utilities/function.objects/negators/binary_negate.pass.cpp:27
     const F f = F(std::logical_and<int>());
+#if TEST_STD_VER <= 17
     static_assert((std::is_same<int, F::first_argument_type>::value), "" );
----------------
Mordante wrote:
> Looking at the similar tests below, this `#if` should be removed due to using `-D_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS`.
Whoops. I think I glanced at `logical_and` too quickly here; this test is actually testing the typedefs on `binary_negate`, not `logical_and`. So you're right, will remove this `#if` (and looks like I already managed to remove the ones below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103753