Page MenuHomePhabricator

[libc++] [P0619] Add _LIBCPP_ABI_NO_BINDER_BASES and remove binder typedefs in C++20.
ClosedPublic

Authored by Quuxplusone on Sat, Jun 5, 10:12 AM.

Details

Summary

What it says on the tin.

This is similar to D103171, and I'm willing for it to be similarly commandeered if @ldionne wants.

Huge merge-conflict for anyone who tries to move std::plus and friends out of <functional> in parallel with this.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Sat, Jun 5, 10:12 AM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSat, Jun 5, 10:12 AM

Update __config and the documentation.

Wmbat accepted this revision.Sat, Jun 5, 3:03 PM

LGTM!

libcxx/include/functional
79

Is it ok to remove the inheritance to binary_function from all arithmetic operators for all C++ versions?

Quuxplusone added inline comments.Sat, Jun 5, 3:39 PM
libcxx/include/functional
79

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

Misplaced the _LIBCPP_SUPPRESS_DEPRECATED_PUSH on bit_not; this should fix the C++03/11 test failures.

Thanks for the clean-up. I see no big issues, but would like to discuss the backwards compatibility flags before approving.

libcxx/include/__functional_base
58

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.

libcxx/test/std/utilities/function.objects/negators/binary_negate.pass.cpp
27

Looking at the similar tests below, this #if should be removed due to using -D_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS.

Quuxplusone added inline comments.Sun, Jun 6, 8:09 AM
libcxx/include/__functional_base
58

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.

libcxx/test/std/utilities/function.objects/negators/binary_negate.pass.cpp
27

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

remove bogus #if per @Mordante's comment

Mordante accepted this revision as: Mordante.Mon, Jun 7, 9:20 AM

LGTM.

libcxx/include/__functional_base
58

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.

ldionne requested changes to this revision.Fri, Jun 11, 4:17 AM

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

libcxx/include/__functional_base
58

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

libcxx/include/functional
79

Yeah, let's not complicate things further.

554

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

560

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?

This revision now requires changes to proceed.Fri, Jun 11, 4:17 AM
Quuxplusone marked 6 inline comments as done.Fri, Jun 11, 7:57 AM
Quuxplusone added inline comments.
libcxx/include/functional
554

These bases are present in C++03 and removed in C++11.
I was looking at other things new-in-'11 and saw them guarded by _LIBCPP_CXX03_LANG, so I copied that here; but that was entirely cargo-cult. I'm happy to remove the guard here, i.e. make it just

#if !defined(_LIBCPP_ABI_NO_BINDER_BASES)

Will do.

@ldionne, my understanding is that these days, _LIBCPP_CXX03_LANG mode is allowed to assume that the compiler supports Foo&& and decltype syntaxes as extensions (but maybe not initializer_list or Args...). Would you be interested in a PR to remove the guards around things like https://github.com/llvm/llvm-project/blob/2d0f1fa/libcxx/include/vector#L703-L712 ?

560

No, that was me not realizing they were deprecated in C++17. I'll add

_LIBCPP_DEPRECATED_IN_CXX17 typedef _Tp result_type;

etc., matching how it's done in "__memory/allocator.h".

Quuxplusone marked 2 inline comments as done.

Address review comments from @ldionne.

Fix deprecation warnings in tests, now that I've correctly deprecated the binder typedefs in C++17.

Also, poke buildbot with my drive-by fixes for the failing GCC tests. (This will be a separate commit, if buildbot's happy with it.)

ldionne accepted this revision.Fri, Jun 11, 2:16 PM

LGTM, but:

  1. CI needs to pass
  2. Please put the GCC-next CI fixes in a different patch, and don't touch the iterator.ops failures (I'm doing that in D103272).
  3. We normally add .verify.cpp tests to check that we've applied the deprecated attribute properly to stuff -- here this should apply to the typedefs. It's kinda nitpicky but I'd like it if you could add them.
libcxx/include/functional
554

Yes, very very interested.

To be precise, what we support is Clang in >= C++03 mode (which supports && & friends as extensions), and GCC in >= C++11 mode (which supports && & friends because it's C++11).

This revision is now accepted and ready to land.Fri, Jun 11, 2:16 PM

Rebase now that libcxx/test/ is back in a better state.