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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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"; }
+};
----------------
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.


================
Comment at: libcxx/include/__expected/bad_expected_access.h:47-49
+  _LIBCPP_HIDE_FROM_ABI const char* what() const noexcept override {
+    return static_cast<bad_expected_access<void> const&>(*this).what();
+  }
----------------
I think I would just drop that since you're not overriding the behavior in any meaningful way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516