[libcxx-commits] [PATCH] D115275: [libc++] Remove _LIBCPP_DEFAULT

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__config:840-844
-#ifdef _LIBCPP_CXX03_LANG
-#  define _LIBCPP_DEFAULT {}
-#else
-#  define _LIBCPP_DEFAULT = default;
-#endif
----------------
Quuxplusone wrote:
> philnik wrote:
> > Quuxplusone wrote:
> > > I have looked at this in the past and decided not to mess with it, because it changes our ABI in C++03 mode. @ldionne, could you take another look with ABI in mind and just confirm whether this still LGTY?
> > > 
> > > Remember `=default`ed members can be trivial, but `{}`'ed members are never trivial, for purposes of ABI.
> > Isn't `_LIBCPP_HIDE_FROM_ABI` supposed to mitiagte that problem?
> No, in that we're talking about a property of the type (whether it's trivially foo-constructible), not a property of the member function per se.
> 
> However, I'm retracting my concern, because I just looked for myself: `_LIBCPP_DEFAULT` is currently used in only the places touched in this PR, and:
> - `std::allocator` already does crazy things with inheritance to get the right trivially-default-constructibility in all modes, so it's not affected in C++03 mode by this patch AFAICT
> - `std::atomic<T>` inherits from `__cxx_atomic_base_impl` which is non-trivial in C++03 mode, so it remains non-trivial in C++03 mode even after this patch
> - `std::error_category` is polymorphic, so it's never trivially default constructible in any mode
> 
> I now say: Ship it! :)
Excellent observation though @Quuxplusone , I hadn't thought about that. I went really fast on the same streak as removing all the other C++03-or-older-compiler workarounds and missed this important fact.

As you say, we're lucky and it's not actually an ABI break, but thanks for paying attention. This sort of ABI break (changing the triviality of widely used types) is the sort of thing that would break in the worst possible ways -- we'd certainly hear about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115275