[libcxx-commits] [PATCH] D93542: [SystemZ][ZOS] Provide CLOCK_MONOTONIC alternative

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/CMakeLists.txt:536
+  if (ZOS)
+    add_definitions(-D_LARGE_TIME_API) # Needed for struct timespec64.
+  endif()
----------------
zibi wrote:
> hubert.reinterpretcast wrote:
> > ldionne wrote:
> > > Can you bury this into `gettod.h`?
> > That may be insufficient if the system headers with the affected interfaces get included before `gettod.h` is reached.
> This has to be defined here so the macro is defined before any system headers is included as stated in the comment. We took this same approach in clang code were similar macros needed to be defined with same requirements.
I'm really unhappy about adding random defines to make platforms behave sanely. Why doesn't z/OS provide `timespec64` when `_LARGE_TIME_API` isn't defined?

Also, you should be using `target_add_definitions()` here instead, to avoid polluting the build flags globally.


================
Comment at: libcxx/src/chrono.cpp:14
+#if defined(__MVS__)
+#include <support/ibm/gettod_zos.h> // gettimeofdayMonotonic
+#endif
----------------
hubert.reinterpretcast wrote:
> Not sure what @ldionne's opinion is here, and sorry for not noticing earlier. There's a `libcxx/src/include/` directory for what I understand to be headers to be used for the library build (as opposed to headers containing interfaces meant to face the library user). I think the header here might fit that first category,
I think it's fine to leave it under `libcxx/include` even though it's only used in the build. I think it would also make sense to move the current contents of `libcxx/src/include` somewhere else to try and colocate headers. I personally don't mind shipping those headers - they are never used until one needs them, but they don't hurt either since they are under `libcxx/include/support`.

@zibi Watch out for an upcoming change where I'll rename `libcxx/include/support` to `libcxx/include/__support` to make sure it doesn't conflict with user-provided directories having the same name. You'll have to do a minor rebasing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93542