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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/DesignDocs/VisibilityMacros.rst:1
 ========================
 Symbol Visibility Macros
----------------
We need to document the idea behind this change in this documentation.


================
Comment at: libcxx/include/__config:574
+#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) && !__has_attribute(__type_visibility__)
+#      define _LIBCPP_TEMPLATE_VIS __attribute__((__visibility__("default")))
 #    else
----------------
Can you add a comment explaining why this is necessary? IIUC, this is only necessary because GCC doesn't implement `__type_visibility__` -- otherwise we would be able to drop this macro entirely.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153658