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

[Raman Tenneti via Phabricator via libc-commits]

rtenneti added a comment.

Jeff and I have made the changes. Please take another look.  Thanks.



================
Comment at: libc/config/linux/api.td:34
+      int tm_yday; // days since January
+      int tm_isdst; // Daylight Saving Time flag
+    };
----------------
MaskRay wrote:
> Seems that tm_isdst is entirely unhandled.
Added a TODO to handle tm_isdst . Also needs to read timezone file. 


================
Comment at: libc/src/time/mktime.cpp:45
+    return (time_t)(-1);
+  time_t tmYearFromBase = t1->tm_year + TimeYearBase;
+
----------------
jeffbailey wrote:
> 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.
Hi Siva and Jeff,
  Added a TODO to handle out of range date and time (it seems mktime in C converts invalid dates to valid dates.  For example  "October 40th is silently converted to November 9th")


================
Comment at: libc/src/time/mktime.cpp:64
+      return (time_t)(-1);
+  }
+
----------------
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.


================
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
----------------
jeffbailey wrote:
> 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.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91551