Details
- Reviewers
ldionne Wmbat zoecarver Mordante - Group Reviewers
Restricted Project - Commits
- rGdc066888bd98: [libc++] [P0619] Add _LIBCPP_ABI_NO_BINDER_BASES and remove binder typedefs in…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
LGTM!
libcxx/include/functional | ||
---|---|---|
79 | Is it ok to remove the inheritance to binary_function from all arithmetic operators for all C++ versions? |
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. |
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). |
LGTM.
libcxx/include/__functional_base | ||
---|---|---|
58 |
True, but AFAIK the binary_function doesn't have the same issue the iterator base class for reverse_iterator.
Valid point. I still have a slight preference to remove them, but not enough the block this patch.
Yeah I did look at what @ldionne did with these macros, the placement of POP seems weird. |
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? |
libcxx/include/functional | ||
---|---|---|
554 | These bases are present in C++03 and removed in C++11. #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". |
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.)
LGTM, but:
- CI needs to pass
- 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).
- 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). |
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.