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

[Jeff Bailey via Phabricator via libc-commits]

jeffbailey added inline comments.


================
Comment at: libc/src/time/mktime.cpp:45
+    return (time_t)(-1);
+  time_t tmYearFromBase = t1->tm_year + TimeYearBase;
+
----------------
sivachandra wrote:
> The standard says, 
> 
>   "The values in time are not checked for being out of range."
> 
> So, is all this checking necessary?
The Single Unix Spec says this:

The mktime() function shall return the specified time since the Epoch encoded as a value of type time_t. If the time since the Epoch cannot be represented, the function shall return the value (time_t)-1 [CX] [Option Start]  and set errno to indicate the error.

So our bug is that we're not setting errno here to EOVERFLOW.


================
Comment at: libc/src/time/mktime.cpp:53
+    if (tmYearFromBase > EndOf32BitEpochYear)
+      return (time_t)(-1);
+    if (t1->tm_mon > 0)
----------------
sivachandra wrote:
> Could we perhaps define a `OVERFLOW_VALUE` instead of doing `(time_t)(-1)` everywhere? You can define it in the implementation header as a `static constexpr` which then allows us to use it in the tests as well.
This should be OUT_OF_RANGE since it serves underflow as well.


================
Comment at: libc/src/time/mktime.cpp:66
+
+  static_assert(sizeof(int) == 4,
+                "ILP64 is unimplemented.  This implementation requires "
----------------
sivachandra wrote:
> Its not clear to me as to what would/should change on a ILP64 platform. Can add a comment explaining this?
We'll add a comment.  In brief: because struct tm is ints, a 32-bit int year can be represented in a 64-bit time_t.  However, a 64-bit int year cannot.  Without a good way to run tests on it, I'd rather just assume a 32-bit int and make sure it catches fire at build time when that's violated.


================
Comment at: libc/test/src/time/mktime_test.cpp:23
+  tm_data->tm_mon = month;        // months since January
+  tm_data->tm_year = year - 1900; // years since 1900
+  tm_data->tm_wday = 0;           // days since Sunday
----------------
sivachandra wrote:
> Except for this line, it doesn't seem like you need this function `initialize_tm_data`. A pattern like this does not work?
> 
> ```
> tm temp{...};
> ...
> temp = {...};
> ```
This is the case where I was hoping we could use a C99/C++20 designated initializer so that it was readable and clean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91551