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

Fri Jun 11 04:17:23 PDT 2021

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

I think it's necessary to have the ABI macro, however I really think we should stop adding those `_LIBCPP_ENABLE_CXX20_REMOVED_XYZ` macros. If someone is opting into using C++20, then they are opting into these removals as well. At that point fixing their code is probably not much harder than flipping that macro.

It's OK if you want to add this macro now for consistency, I can make a separate patch to remove a bunch of them (and incidentally own the responsibility if people get mad).

================
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:
> 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.
> 
> 
Basically, you want to push-pop around the usage of `binary_function<...>` only, however GCC will complain if either appears directly before/after the inheritance (before the opening brace of the class definition).

================
Comment at: libcxx/include/functional:555
+#if defined(_LIBCPP_CXX03_LANG) || !defined(_LIBCPP_ABI_NO_BINDER_BASES)
+    : binary_function<_Tp, _Tp, _Tp>
+#endif
----------------
What Standard removed those bases? Is it C++11 or C++14?

Using `_LIBCPP_CXX03_LANG` doesn't look right to me. That macro only means that the compiler supports C++03 language feature, it is not related to the standard version that libc++ tries to implement (which is always C++11 or above).

================
Comment at: libcxx/include/functional:561
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
+    typedef _Tp result_type;
+    typedef _Tp first_argument_type;
----------------
Those typedefs are deprecated in C++17. I assume you did not mark them as deprecated since if someone is opting into them, they are probably not wanting to fight agains the deprecation warning?

================
Comment at: libcxx/include/functional:79
 template <class T> // <class T=void> in C++14
-struct plus : binary_function<T, T, T>
-{
+struct plus {
     T operator()(const T& x, const T& y) const;
----------------
Quuxplusone wrote:
> Wmbat wrote:
> > Is it ok to remove the inheritance to `binary_function` from all arithmetic operators for all C++ versions?
> (Note that here we're talking only about the synopsis comment.) Personally I think this simplification is fine, but I'll take suggestions from Louis if he really wants to complicate matters. E.g. we could do
> ```
> template <class T> // <class T=void> in C++14
> struct plus {  // derives from binary_function<T, T, T> before C++11
> ```
> but that seems more costly than beneficial IMHO. Honestly I'd welcome a suggestion to just write
> ```
> template <class T=void>
> struct plus {
> ```
> at this point in the century. We never bothered to say `// constexpr in C++11` or whatever, either.
Yeah, let's not complicate things further.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103753