[libcxx-commits] [PATCH] D118620: [SystemZ][z/OS] Build several exception derived classes as a separate library

Mon Nov 7 07:28:22 PST 2022

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

I don't really like the state this leaves `variant`, `any` and friends in when exception vtables aren't compiled into the dylib.
There's no way to know only from the headers that using any of these types will result in linker errors.

Yes, is the Z/OS case you provide these symbols via a different means, but that's not clear anywhere in the  CMake files as written.

I think the source changes look acceptable, but the changes to the build system may not be changes we want in upstream. There has been a enormous amount of work to simply and remove all sorts of cryptic build-time switches in order to make our CMake files understandable, and I see this change as going backwards in that respect.

================
Comment at: libcxx/include/__memory/shared_ptr.h:50

+_LIBCPP_BEGIN_NAMESPACE_EXCEPTION
+
----------------
I don't think we want to formalize this different exception namespace. Especially as a versioned one (which it is by default).

The correct thing to do is place these types directly in namespace `std`, so their mangling never changes. `bad_weak_ptr` should have never gone in the versioned namespace, but that mistake was made, and now we're stuck with it (for now).

I'm OK with this change, but lets be verbose and do the ugly `#ifdef` blocks and namespace switching directly around `bad_weak_ptr`.

================
Comment at: libcxx/src/CMakeLists.txt:138
+
+if (NOT ZOS)
+  list(APPEND LIBCXX_SOURCES ${LIBCXX_EXCEPTIONS_SOURCE})
----------------
This should be gated on the other CMake switch you're introducing, no?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118620