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

[Zibi Sarbino via Phabricator via libcxx-commits]

zibi marked 3 inline comments as done.
zibi added inline comments.


================
Comment at: libcxx/CMakeLists.txt:536
+  if (ZOS)
+    add_definitions(-D_LARGE_TIME_API) # Needed for struct timespec64.
+  endif()
----------------
ldionne wrote:
> zibi wrote:
> > hubert.reinterpretcast wrote:
> > > ldionne wrote:
> > > > 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.
> > > It seems reasonable to provide `timespec64` through `time.h` (which provides `timespec`). However, `time.h` is a standard header, and `timespec64` does not match any of the standardized forms of identifiers that are reserved (or at least it does not clearly look like a reserved identifier to me). It is a valid design choice to use opt-in (as opposed to opt-out) macros for controlling extensions.
> > Believe me, I'm totally with you on this but, unfortunately I have no control over visibility of `timespec64`.  We could hide this in one of the wrapper tools but this would not be fare for not being able to build library using just the repo. 
> > 
> > However the good news is we might remove the entire patch once we get one we get monotomic clock shipped with OS.
> It's a valid design choice, but it's both inconvenient and inconsistent with what other platforms do. Anyway, this is OK but we need to use `target_add_definitions`.
> It's a valid design choice, but it's both inconvenient and inconsistent with what other platforms do. Anyway, this is OK but we need to use `target_add_definitions`.

Are you suggesting to define new macro `target_add_definitions` and use it instead of  `add_definitions`? Just to make sure you want to define `'_LARGE_TIME_API ' only for building `chrono.cpp.p`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93542