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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Mostly LGTM. I have left a few nits along with a not-a-nit comment.

>From offline discussions, I understand you are preparing follow ups. It should be OK to land WiP pieces as long as you have clear TODOs about what is missing.



================
Comment at: libc/src/time/mktime.cpp:64
+      return (time_t)(-1);
+  }
+
----------------
jeffbailey wrote:
> rtenneti wrote:
> > sivachandra wrote:
> > > 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`.
> > Add a TODO to investigate the above idea.
> Is the TODO still necessary with the creation of outOfRange() that sets errno and returns -1?
Along with the above comment about checking for invalid input, what I meant was this: As we start accumulating the seconds, we should be able to check if the next amount of seconds to be added can lead to an overflow. If it does, return the overflow value. If not keep accumulating. The benefit is that, we don't have to validate every input, and also do need the special cases for `sizeof(time_t) == 4`.


================
Comment at: libc/src/time/mktime.cpp:42
+  llvmlibc_errno = EOVERFLOW;
+  return (time_t)(-1);
+}
----------------
Nit: Use `static_cast` instead of C-style casts, similar to how you do in tests. Even better, define another constant like this:

```
constexpr time_t OutOfRangeReturnValue = -1;
```


================
Comment at: libc/src/time/mktime.cpp:45
+
+time_t LLVM_LIBC_ENTRYPOINT(mktime)(struct tm *t1) {
+  // Unlike most C Library functions, mktime doesn't just die on bad input.
----------------
Can you add TODOs for the pieces missing in this implementation? Just rephrase what you have in the commit message as a TODO.


================
Comment at: libc/test/src/time/mktime_test.cpp:30
+
+static inline time_t mktime(struct tm *tm_data, int year, int month, int mday,
+                            int hour, int min, int sec) {
----------------
Nit: To avoid confusions, can you rename this function to `call_mktime`?


================
Comment at: libc/test/src/time/mktime_test.cpp:44
+  struct tm tm_data;
+  EXPECT_EQ(mktime(&tm_data, 0, 0, 0, 0, 0, -1), static_cast<time_t>(-1));
+  EXPECT_EQ(mktime(&tm_data, 0, 0, 0, 0, 0, 60), static_cast<time_t>(-1));
----------------
Nit: Define a global perhaps to avoid repeated listings of `static_cast`. Something like:

```
static constexpr time_t OutOfRangeReturnValue = -1;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91551