[libc-commits] [PATCH] D96684: Changes to mktime to handle invalid dates, overflow and underflow andcalculating the correct date and thenumber of seconds even if invalid datesare passed as arguments.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/time/mktime.cpp:18
 
-constexpr int SecondsPerMin = 60;
-constexpr int MinutesPerHour = 60;
-constexpr int HoursPerDay = 24;
-constexpr int DaysPerWeek = 7;
-constexpr int MonthsPerYear = 12;
-constexpr int DaysPerNonLeapYear = 365;
-constexpr int TimeYearBase = 1900;
 constexpr int EpochYear = 1970;
 // The latest time that can be represented in this form is 03:14:07 UTC on
----------------
You can probably move these constants also to the class `TimeConstants` suggested above..


================
Comment at: libc/src/time/mktime.h:17
 
+constexpr int SecondsPerMin = 60;
+constexpr int SecondsPerHour = 3600;
----------------
The implementation header should not leak any implementation details. These constants here aren't really implementation details, but lets use a different approach:

  # Create a header only library by name `time_utils` with a header file `time_utils.h`. You have done that already so I am essentially suggesting a reorg of the file.
  # Use a nested namespace `__llvm_libc::time_utils`.
  # In that file, create a struct by name `__llvm_libc::time_utils::TimeConstants` and list these constants as static members. You should do the same with the other constants you have listed in `time_utils.cpp`.




================
Comment at: libc/src/time/mktime.h:27
 
+time_t outOfRange();
+
----------------
Likewise, you should make this function `static inline` and define it in `time_utils.h` in the nested namespace `__llvm_libc::time_utils`.


================
Comment at: libc/src/time/time_utils.cpp:39
+
+int64_t updateFromSeconds(int64_t total_seconds, struct tm *tm) {
+  // Days in month starting from March in the year 2000.
----------------
Is there a reason why this should be in a separate object file? Can this be not be listed as `static` in `mktime.cpp`?


================
Comment at: libc/test/src/time/mktime_test.cpp:19
 
 static constexpr time_t OutOfRangeReturnValue = -1;
 
----------------
You can define this constant in `time_utils.h` in the class `TimeConstants` as:

```
namespace __llvm_libc {
namespace time_utils {

struct TimeConstants {
  ...
  // If more functions beyond mktime will use this return value, then don't add the "MkTime" prefix.
  static constexpr time_t MkTimeOutOfRangeReturnValue = -1;
};

} // namespace time_utils
} // namespace __llvm_libc
```


================
Comment at: libc/test/src/time/mktime_test.cpp:44
+  /* year, month, .. are all actual date and time */
+  void check_mktime(struct tm *tm_data, int year, int month, int mday, int hour,
+                    int min, int sec, int wday, int yday) {
----------------
Ideally, we should implement a matcher : https://github.com/llvm/llvm-project/blob/main/libc/utils/UnitTest/LibcTest.h#L55
Example matcher: https://github.com/llvm/llvm-project/blob/main/libc/test/ErrnoSetterMatcher.h#L22.

A matcher will help us show better failure messages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96684