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

[Zibi Sarbino via Phabricator via libcxx-commits]

zibi added a comment.

@EricWF Thank you for the review.

@ldionne Can you weigh on this as requested by Eric?



================
Comment at: libcxx/include/__memory/shared_ptr.h:50
 
+namespace std {
+#else
----------------
EricWF wrote:
> Let's create an ABI macro, since other users might want to have the `bad_weak_ptr` exception too, that will make this code more readable and then you can just turn it on for your platform.
> 
> Sorry for not suggesting that earlier.
@ldionne Do you agree with Eric's suggestion? 
Also I believe we HAD a macro before but it was suggested to remove it and just use `namespace std` directly.  If the recommendation is to go back to macro idea I wonder where should it go since it is needed here as well in `libcxx/src/exceptions.cpp`. 


================
Comment at: libcxx/src/CMakeLists.txt:138
+
+if (NOT ZOS)
+  list(APPEND LIBCXX_SOURCES ${LIBCXX_EXCEPTIONS_SOURCE})
----------------
EricWF wrote:
> zibi wrote:
> > EricWF wrote:
> > > This should be gated on the other CMake switch you're introducing, no?
> > Not really, we build the library multiple times to support ASCII and EBCDIC but the exception is being build  only once in EBCDIC encoding.  Using other CMake switch will build cxx with and without exception. On z/OS we don't want to build exception as part of cxx. This is the fundamental reason why we doing this change.
> Right, then these things should go into libc++abi. Not a third separate library.
> 
> 
See the discussion below between Fanbo and Loius why we created 3rd library. We really cannot undo this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118620