[libcxx-commits] [PATCH] D140714: [libc++][NFC] Rename basic_istream_view::__iterator to __basic_istream_view_iterator

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/istream_view.h:41
+  requires default_initializable<_Val> && __stream_extractable<_Val, _CharT, _Traits>
+class __basic_istream_view_iterator;
+
----------------
philnik wrote:
> Mordante wrote:
> > Same for the definition.
> What does this actually do and how can we test it? We use the macro quite a bit, but not in any consistent way AFAICT.
Looking at the documentation and the definition of the macro, it applies the `__type_visibility__(default)` attribute to the type. This gives the vtable and the RTTI of the type (if any) default visibility.

This type doesn't have a vtable. The only scenario I can see this mattering in would be if someone throws a `__basic_istream_view_iterator` from a dylib and tries to catch it from an executable that uses that dylib. If the RTTI doesn't have default visibility, the RTTI generated in the executable and the RTTI generated in the dylib won't be coalesced by the dynamic linker, so we'll end up with two copies of the RTTI in the process. This means that e.g. the `type_info::name()` address would differ depending on whether you're inside the dylib or the executable, etc. Since some ABIs (the Itanium ABI) assume that the RTTI is unique within the process, this could lead to a situation where you are e.g. incapable of catching the object that was thrown from the dylib, since from the runtime's perspective they would be different types. See the `Unique` implementation of RTTI inside `libcxx/include/typeinfo`.

Caveat: This is kinda complicated and I'm just trying to work out the implications of this macro right now. TBH I've always found our visibility macros to be overcomplicated and have had on my mind to refactor them. For now I'd suggest that it's acceptable to not use this macro on this type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140714