[libc-commits] [PATCH] D134095: Implement nanosleep per https://pubs.opengroup.org/onlinepubs/009695399/basedefs/time.h.html

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libc/include/llvm-libc-types/CMakeLists.txt:38
 add_header(struct_tm HDR struct_tm.h)
+add_header(struct_timespec HDR struct_timespec.h)
 add_header(thrd_start_t HDR thrd_start_t.h)
----------------
Since this depends on `time_t.h`, we should add a `DEPENDS .time_t`.


================
Comment at: libc/include/llvm-libc-types/struct_timespec.h:17
+  time_t tv_sec; /* Seconds.  */
+  long tv_nsec;  /* Nanoseconds.  */
+};
----------------
I think this definition is correct as per the POSIX spec. But, since we are passing structures of this shape to the Linux syscall, we should probably add a comment or TODO in `src/time/nanosleep.cpp` explaining why this definition is sufficient or incomplete.


================
Comment at: libc/spec/stdc.td:17
       "assert.h",
       [
           Macro<"static_assert">,
----------------
Can you do the changes in the file separately as an obvious change?


================
Comment at: libc/src/time/mktime.h:1
 //===-- Implementation header of mktime -------------------------*- C++ -*-===//
 //
----------------
Can you make the changes in this file also separately?


================
Comment at: libc/test/src/time/nanosleep_test.cpp:21
+
+TEST(LlvmLibcNanosleep, sleep) {
+  using __llvm_libc::testing::ErrnoSetterMatcher::Succeeds;
----------------
Call it `SmokeTest` may be? Also, move the above comment inside this test block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134095