[libcxx-commits] [PATCH] D153658: [libc++] Make everything in namespace std have default type visibility and hidden visibility and remove _LIBCPP_ENUM_VIS

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__config:656-657
 // clang-format off
-#  define _LIBCPP_BEGIN_NAMESPACE_STD namespace std { inline namespace _LIBCPP_ABI_NAMESPACE {
+#  define _LIBCPP_BEGIN_NAMESPACE_STD namespace _LIBCPP_TYPE_VISIBILITY_DEFAULT _LIBCPP_HIDDEN std {                   \
+                               inline namespace _LIBCPP_ABI_NAMESPACE {
 #  define _LIBCPP_END_NAMESPACE_STD }}
----------------
ldionne wrote:
> This change will have the following non-trivial effects:
> 1. Everything in namespace `std` will now have default type-visibility. Previously, only the things marked with a visibility macro would get that treatment. Why is that okay? Can this bite users or us?
> 2. Everything in namespace `std` will now have hidden visibility unless overridden. Previously, that was our intent, but we might not have been doing it 100% consistently so this might actually have an impact on what symbols become exported when folks use our headers. Do we have an idea of which symbols this impacts, and have you thought about the potential user impact of this?
> 
> Let's document that in the commit message (and also in a release note if we foresee user impact).
1) Every public type is supposed to have default type visibility to make it possible to throw things across library boundaries and have RTTI work (that is required for e.g. `std::any`). While the usefulness is questionable for many types (e.g. `remove_const` and friends), it is technically allowed to get RTTI for all types. For library-internal types, it should be fine, since RTTI won't ever get actually generated for them, making it irrelevant what visibility the symbols have. This will be a user-visible change insofar as it will fix any missing visibility attributes on classes, but I don't think that will have a big impact.

2) This should™ only impact global variables and functions which don't have any kind of visibility annotation. Given that almost all functions already have visibility annotations, this shouldn't make much of an impact there. The global variables will be duplicated across libraries with this change, but I don't think that's much of a problem. This should only be noticeable by checking the address of the variable, since any global variables I'm aware of are `constexpr` or at least `const`. Almost nobody does that, since it's not interesting other than checking for strict conformance, so I wouldn't consider that much of a problem either. This also already happens when `-fvisibility=hidden` is used, which is the case for people who care about their ABI surface.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153658