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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

In D153658#4599282 <https://reviews.llvm.org/D153658#4599282>, @ldionne wrote:

> This LGTM with passing CI, updated commit message and the release note.
>
> For most people who compile with `-fvisibility=default` (or nothing at all), this is a no-op since everything in the library already had default visibility. For users who build with `-fvisibility=hidden`, there used to be a bunch of types that we forgot to apply `type_visibility(default)` or `visibility(default)` to, and those would previously be hidden. After this change, their type_visibility would become `default` (as it should) on Clang (but not on GCC).

I don't think this change was thought through.

- This is a major issue for people who build and ship their own shared libraries.
- Iterating over long symbol lists is an issue, and can cause really yucky hash collisions in the dynamic linker loader.
- This isn't just about types. It's about functions. And now every single weak function template that's instantiated as part of a users library build

Based on the comments in this change, it's not clear to me you considered the most common use cases for `-fvisibility=hidden`.
Could you explain to me what affect you believe this change has on other shared libraries?



================
Comment at: libcxx/docs/ReleaseNotes/18.rst:82
+  ``-fvisbility=hidden`` may notice that additional type infos from libc++ are being exported from their ABI. This is
+  the correct behavior in almost all cases since exporting the RTTI is required for these types to work properly with
+  dynamic_cast, exceptions and other mechanisms across binaries. However, if you intend to use libc++ purely as an
----------------
A lot of this is incorrect.

-fvisibility-hidden is for when your



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153658