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

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxx/src/include/internal_threading_support.h:27
+#ifndef _LIBCPP_HAS_NO_THREADS
+#  include <cassert>
+
----------------
EricWF wrote:
> I think we're very trying to avoid including `<cassert>` anywhere within libc++ headers.
Even for internal headers? (Note that this file is under `libcxx/src/include` rather than `libcxx/include`)


================
Comment at: libcxx/src/include/internal_threading_support.h:30
+#  if defined(__MVS__)
+static inline bool __libcpp_is_threading_api_enabled();
+
----------------
EricWF wrote:
> Why `static inline`? 
To prevent this symbol from getting exported as part of the library (it shouldn't ever get invoked by user code and is purely for internal use within libcxx/libcxxabi source), and also to prevent duplicate definitions as a result of using this header in both `libcxx` and `libcxxabi`.


================
Comment at: libcxx/src/include/internal_threading_support.h:35
+#  else
+static inline _LIBCPP_CONSTEXPR bool __libcpp_is_threading_api_enabled();
+
----------------
EricWF wrote:
> These declarations are different every time they appear..
I'm not sure what you mean?

Since there's no standard way of checking whether the base threading library (e.g. pthreads) is available, each platform which needs to support runtime-dependent threading will have to write their own implementation of these functions (line 40-54). For example, AIX would change the `#if` on line 29 to `#if defined(__MVS__) || defined(_AIX)`, and add it's own implementation before line 49 (guarded by an `# elif defined(_AIX)`).

Since each platform will need to provide their own implementation, we've provided a separate forward declaration to ensure the signature is always the same across all platforms (line 29-38). However, on platforms which don't have runtime-dependent threading (i.e. they know at the time we compile libcxx/libcxxabi whether a thread library like pthreads is available) the default, trivial implementation we provide should ideally declare these functions as `constexpr`. To do this we need to provide two separate versions of the forward declaration.



================
Comment at: libcxx/src/include/internal_threading_support.h:62
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace internal_thread {
+
----------------
EricWF wrote:
> This would need to be a reserved identifier.
Probably a good idea to use a reserved identifier anyways, but this is internal to libcxx/libcxxabi, so it's impossible for this to produce any negative interaction with user code. Note that every symbol in this file is `static inline`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120348