Re: [PATCH] Add timegm POSIX call

On Wed, Aug 15, 2018 at 10:01 AM, Craig Howland <[email protected]>
wrote:

> On 08/13/2018 09:17 PM, Andrew Russell via newlib wrote:
>
>> From: Andrew Russell <[email protected]>
>> Date: Fri, 10 Aug 2018 12:14:18 -0700
>> Subject: [PATCH 1/4] Start of mktime.c copy to timegm.c
>>
>> I am proposing to add the timegm POSIX call to
>> Newlib. Part of this refactors some of the code in libc/time/local.h and
>> libc/time/mktime.c, per this discussion:
>>
>> https://sourceware.org/ml/newlib/2018/msg00186.html
>>
>> Thanks,
>> Andrew
>>
>> ...
>>
>> diff --git a/newlib/libc/include/time.h b/newlib/libc/include/time.h
>> index a2efcc15e..8440ecb3c 100644
>> --- a/newlib/libc/include/time.h
>> +++ b/newlib/libc/include/time.h
>> @@ -56,6 +56,7 @@ struct tm
>>   clock_t    clock (void);
>>   double    difftime (time_t _time2, time_t _time1);
>>   time_t    mktime (struct tm *_timeptr);
>> +time_t    timegm (struct tm *_timeptr);
>>
> This prototype should be gated by the appropriate-version GNU extension
> #if.  (According to the Linux timegm() man page the gate is _BSD_SOURCE ||
> _SVID_SOURCE (although the Newlib in-header version might be different).
> But given it is called a GNU extension, you'd think _GNU_SOURCE would also
> be there.)
>
>>   time_t    time (time_t *_timer);
>>   #ifndef _REENT_ONLY
>>   char   *asctime (const struct tm *_tblock);
>>
>> ...
>>
>> diff --git a/newlib/libc/time/mktime.c b/newlib/libc/time/mktime.c
>> index 02032599a..ab47f8614 100644
>> --- a/newlib/libc/time/mktime.c
>> +++ b/newlib/libc/time/mktime.c
>> @@ -11,6 +11,8 @@
>>    * represented, returns the value (time_t) -1.
>>    *
>>    * Modifications: Fixed tm_isdst usage - 27 August 2008 Craig Howland.
>> + *                      Refactor code from mktime.c to shared internal
>> + *                      functions. - 17 July 2018 Andrew Russell.
>>    */
>>
>>   /*
>> @@ -39,157 +41,22 @@ result is the time, converted to a <<time_t>> value.
>>   PORTABILITY
>>   ANSI C requires <<mktime>>.
>>
>> -<<mktime>> requires no supporting OS subroutines.
>> +<<timegm>> requires no supporting OS subroutines.
>>
> Spurious change, as this file is still mktime.
>
>>   ...
>> diff --git a/newlib/libc/time/timegm.c b/newlib/libc/time/timegm.c
>>
> (This diff as presented does not make sense from the point of view that
> this is a new file.)
>
>> index 02032599a..3299ce228 100644
>> --- a/newlib/libc/time/timegm.c
>> +++ b/newlib/libc/time/timegm.c
>> @@ -1,8 +1,8 @@
>>   /*
>> - * mktime.c
>> + * timegm.c
>>    * Original Author: G. Haley
>>    *
>> - * Converts the broken-down time, expressed as local time, in the
>> structure
>> + * Converts the broken-down time, expressed as UTC time, in the structure
>>    * pointed to by tim_p into a calendar time value. The original values
>> of the
>>    * tm_wday and tm_yday fields of the structure are ignored, and the
>> original
>>    * values of the other fields have no restrictions. On successful
>> completion
>> @@ -11,25 +11,27 @@
>>    * represented, returns the value (time_t) -1.
>>    *
>>    * Modifications: Fixed tm_isdst usage - 27 August 2008 Craig Howland.
>>
> Can get rid of this fix tm_isdst comment, as not in timegm, but in mktime.
> The comments that make the man page here should probably include the same
> "These functions are nonstandard GNU extensions that are also present on
> the BSDs.  Avoid their use; see NOTES." statement that appears in the
> timegm/timelocal Linux man page (with a small tweak for being only 1
> function instead of two).
>

+1 Critical to have good documentation at the top.

> ...
>>
>>   #include <stdlib.h>
>> @@ -58,11 +60,8 @@ static const int DAYS_IN_MONTH[12] =
>>   static const int _DAYS_BEFORE_MONTH[12] =
>>   {0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};
>>
> Someone had suggested in the thread that DAYS_IN_MONTH be changed to char
> to save space.  How about int_least16_t to (possibly) save space?
>
>> ...
>>
>> -time_t
>> -mktime (struct tm *tim_p)
>> +void
>> +__set_tm_wday (long days, struct tm *tim_p)
>> +{
>> +  if ((tim_p->tm_wday = (days + 4) % 7) < 0)
>> +    tim_p->tm_wday += 7;
>> +}
>> +
>> +/* returns either 0 or 1 */
>> +static
>> +int
>> +__is_leap_year (int year)
>> +{
>> +  return (year % 4) == 0 && ((year % 100) != 0 || (((year / 100) & 3)
>> == (-(YEAR_BASE / 100)  & 3)));
>>
> How about "(year & 3) == 0" instead of the % to save time? (Should not
> matter with a good optimizer, but it is not necessarily on.)
>
>> ...
>> +time_t
>> +timegm (struct tm *tim_p)
>> +{
>> +  time_t tim = __timegm_internal(tim_p);
>> +  long days = tim / SECSPERDAY;
>>
>> -  /* reset isdst flag to what we have calculated */
>> -  tim_p->tm_isdst = isdst;
>> +  /* set isdst flag to 0 since we are in UTC */
>> +  tim_p->tm_isdst = 0;
>>
>>     /* compute day of the week */
>> -  if ((tim_p->tm_wday = (days + 4) % 7) < 0)
>> -    tim_p->tm_wday += 7;
>> -
>> +  __set_tm_wday(days, tim_p);
>> +
>>     return tim;
>>   }
>> +
>> --
>> 2.18.0.597.ga71716f1ad-goog
>>
>>
> I have not been able to try testing it, yet, but there are a few thoughts
> based on examination.  See inline above as well as some here.
>
> I suggest that the new timegm.c file should contain only the new timegm()
> function and that the supporting functions all belong in mktime.c.  This
> comes closest to preserving the same size on existing implementations,
> which otherwise would then require the timegm object to be linked; the new
> timegm object will only get linked for applications which call it.  (Not
> that the timegm() function, itself, is large, but it is the new extension.)
>
> Craig
>