[libc-commits] [PATCH] D98467: This introduces gmtime to LLVM libc, based on C99/C2X/Single Unix Spec.

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

Couple of questions inline. Feel free to submit after answering/addressing them.



================
Comment at: libc/src/time/CMakeLists.txt:8
+  DEPENDS
+    libc.utils.CPP.standalone_cpp
+)
----------------
Why do we need `standalone_cpp`?


================
Comment at: libc/src/time/gmtime.cpp:18
+LLVM_LIBC_FUNCTION(struct tm *, gmtime, (const time_t *timer)) {
+  // TODO(rtenneti): This method is not thread safe.
+  static struct tm tm_out;
----------------
Does any standard require thread safety? If yes, will making the below declaration thread-local solve the problem?


================
Comment at: libc/test/src/time/gmtime_test.cpp:23
+
+TEST(LlvmLibcGmTime, GmTimeTestsOutOfRange) {
+  time_t seconds = 1 + INT_MAX * static_cast<int64_t>(
----------------
Why do we need the `GmTimeTests` prefix? Doesn't the suite name `LlvmLibcGmTime` already convey that it is a `gmtime` test?


================
Comment at: libc/test/src/time/mktime_test.cpp:53
 
-TEST(LlvmLibcMkTime, MkTimesInvalidSeconds) {
+TEST(LlvmLibcMkTime, MkTimeTestsInvalidSeconds) {
   struct tm tm_data;
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98467