[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__expected/bad_expected_access.h:39
+public:
+  _LIBCPP_HIDE_FROM_ABI const char* what() const noexcept override { return "bad access to std::expected"; }
+};
----------------
ldionne wrote:
> This is not what we usually do for exception classes -- we put their vtable in the dylib via a key function to avoid getting one vtable per TU and having the dynamic linker do a bunch of work. And then we use availability attributes to catch misuse at compile-time.
> 
> That being said, the way this has been designed (by using a class template below) means that we'll already have a profusion of these vtables in TUs, and the dynamic linker will already have a bunch of work to do. So I'm not sure it is worth hiding the `<void>` specialization in the dylib, given that it adds deployment target restrictions.
> 
> So I'd leave it as-is, but I think the rationale above is important to keep in mind.
Could we add the rationale as a comment above `bad_expected_access<void>`? You probably have to add `_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wweak-vtables")`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516