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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

LGTM.



================
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;
----------------
Quuxplusone wrote:
> 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.
> 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.

True, but AFAIK the `binary_function` doesn't have the same issue the `iterator` base class for `reverse_iterator`.

> 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.

Valid point. I still have a slight preference to remove them, but not enough the block this patch.

> 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.

Yeah I did look at what @ldionne did with these macros, the placement of POP seems weird.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103753