[libc-commits] [PATCH] D91551: Initial commit of mktime.

[Fangrui Song via Phabricator via libc-commits]

MaskRay added a comment.

Drive-by: POSIX.1-2017: "Local timezone information shall be set as though mktime() called tzset()." Is "TZ" handled in this patch?



================
Comment at: libc/config/linux/api.td:34
+      int tm_yday; // days since January
+      int tm_isdst; // Daylight Saving Time flag
+    };
----------------
Seems that tm_isdst is entirely unhandled.


================
Comment at: libc/src/time/mktime.cpp:11
+#include "src/__support/common.h"
+
+namespace __llvm_libc {
----------------
Drive-by: LLVM Coding Standards talk about the use of anonymous namespaces: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions


================
Comment at: libc/src/time/mktime.cpp:15
+namespace {
+constexpr int SecondsPerMin = 60;
+constexpr int MinutesPerHour = 60;
----------------
namespace-scope const/constexpr variables are automatically of internal linkage. You don't need an anonymous namespace (which is also discouraged) in this case.


================
Comment at: libc/src/time/mktime.cpp:36
+
+constexpr int LeapYearDaysByMonth[] = {0 /* Jan */, 31,  60,  91,  121, 152,
+                                       182,         213, 244, 274, 305, 335};
----------------
Have two arrays for leap/nonleap wastes space (libc.a space matters a lot). Having one array plus leap adjustment is sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91551