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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
Herald added a project: All.


================
Comment at: libcxx/include/__config:801-804
+#  else
+#    define _LIBCPP_BEGIN_NAMESPACE_STD_EXCEPTION _LIBCPP_BEGIN_NAMESPACE_STD
+#    define _LIBCPP_END_NAMESPACE_STD_EXCEPTION _LIBCPP_END_NAMESPACE_STD
+#  endif
----------------
It seems to me that we *always* want the namespace for exceptions to be `namespace std` only, without the ABI namespace.

I think it only falls down for `bad_weak_ptr`, which we seem to have defined in `std::__1` instead of `std::`. Is that correct?


================
Comment at: libcxx/src/CMakeLists.txt:296
 
+if (LIBCXX_EXCEPTION_ZOS_BUILD)
+  add_library(cxx_exception SHARED ${exclude_from_all} ${LIBCXX_EXCEPTION_SOURCES} ${LIBCXX_HEADERS})
----------------
I think there's something greater trying to emerge here. We already define some exception types inside libc++abi in namespace `std`. We should have a consistent approach with all those. In fact, perhaps what we want is to create either a static archive or a shared library with all those exception types, from libc++ and libc++abi. The default configuration would be to statically link that library into libc++, but other configurations could be possible, like leaving it be its own dylib (your use case).


================
Comment at: libcxx/src/exceptions/exception_any.cpp:9
+
+#include "any"
+
----------------
Please use `<any>` instead of `"any"`. Also applies below.


================
Comment at: libcxx/src/exceptions/exception_any.cpp:11
+
+namespace std {
+const char* bad_any_cast::what() const noexcept { return "bad any cast"; }
----------------
I think this should be using `_LIBCPP_BEGIN_NAMESPACE_STD_EXCEPTION`. Same for others below.


================
Comment at: libcxx/src/exceptions/exception_variant.cpp:13
 
-const char* bad_variant_access::what() const noexcept {
-  return "bad_variant_access";
-}
+const char* bad_variant_access::what() const noexcept { return "bad_variant_access"; }
 
----------------
Please don't change the formatting needlessly. Applies everywhere.


================
Comment at: libcxx/utils/libcxx/test/config.py:377
             self.cxx.link_flags += ['-lc++']
+            if self.target_info.is_zos():
+                self.cxx.link_flags += ['-lc++_exception']
----------------
Please define a configuration file in the likes of `libcxx/test/configs/apple-libc++-shared.cfg.in` for testing. We're trying to move away from this `config.py`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118620