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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/time/mktime.cpp:45
+    return (time_t)(-1);
+  time_t tmYearFromBase = t1->tm_year + TimeYearBase;
+
----------------
The standard says, 

  "The values in time are not checked for being out of range."

So, is all this checking necessary?


================
Comment at: libc/src/time/mktime.cpp:53
+    if (tmYearFromBase > EndOf32BitEpochYear)
+      return (time_t)(-1);
+    if (t1->tm_mon > 0)
----------------
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.


================
Comment at: libc/src/time/mktime.cpp:64
+      return (time_t)(-1);
+  }
+
----------------
Could we do this overflow check in a more general fashion? For example, we can find a `maxYears` that can fit in `TIME_T_MAX` (I am making up `TIME_T_MAX`). After that, assuming valid inputs for other fields, we can progressively check if the result will fit in `time_t`.


================
Comment at: libc/src/time/mktime.cpp:66
+
+  static_assert(sizeof(int) == 4,
+                "ILP64 is unimplemented.  This implementation requires "
----------------
Its not clear to me as to what would/should change on a ILP64 platform. Can add a comment explaining this?


================
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
----------------
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 = {...};
```


================
Comment at: libc/test/src/time/mktime_test.cpp:33
+  time_t result = -1;
+  EXPECT_EQ(secs, result);
+
----------------
You can inline lines 31 to 33 to:

```
EXPECT_EQ(__llvm_libc::mktime(&temp), OVERFLOW_VALUE);
```

And use the same pattern at other places where relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91551