[libcxx-commits] [PATCH] D120348: [libcxx][SystemZ][ POSIX(OFF) support on z/OS

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: libcxx/src/include/internal_threading_support.h:27
+#ifndef _LIBCPP_HAS_NO_THREADS
+#  include <cassert>
+
----------------
I think we're very trying to avoid including `<cassert>` anywhere within libc++ headers.


================
Comment at: libcxx/src/include/internal_threading_support.h:30
+#  if defined(__MVS__)
+static inline bool __libcpp_is_threading_api_enabled();
+
----------------
Why `static inline`? 


================
Comment at: libcxx/src/include/internal_threading_support.h:35
+#  else
+static inline _LIBCPP_CONSTEXPR bool __libcpp_is_threading_api_enabled();
+
----------------
These declarations are different every time they appear..


================
Comment at: libcxx/src/include/internal_threading_support.h:62
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace internal_thread {
+
----------------
This would need to be a reserved identifier.


================
Comment at: libcxx/src/include/internal_threading_support.h:76
+}
+
+static inline bool __libcpp_recursive_mutex_trylock(__libcpp_recursive_mutex_t* __m) {
----------------
Why not just use weak functions to allow different definitions, which can live outside of mainline libc++, to be shimmed in at link/load time? 


================
Comment at: libcxx/src/include/internal_threading_support.h:178
+    // __libcpp_is_threading_api_enabled() to return false.
+    assert(false);
+#  endif
----------------
No `assert`. Use one of the abort methods. 


================
Comment at: libcxxabi/src/cxa_exception_storage.cpp:96
+        // If threads are disabled at runtime, revert to single-threaded implementation.
+        if (!__libcpp_is_threading_api_enabled()) {
+            static __cxa_eh_globals eh_globals;
----------------

This seems like performance sensitive code. Are we cool with a branch here?

Also, are we even allowed to use function local statics in this function? 



================
Comment at: libcxxabi/test/test_exception_storage.pass.cpp:19
 
 void *thread_code (void *parm) {
+  size_t* result = (size_t*)parm;
----------------
This diff is hard to read because of all the whitespace changes.
What has actually changed here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120348