This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [PATCH] Add timegm POSIX call
On Wed, Aug 15, 2018 at 10:01 AM, Craig Howland <howland@lgsinnovations.com>
wrote:
> On 08/13/2018 09:17 PM, Andrew Russell via newlib wrote:
>
>> From: Andrew Russell <ahrussell@google.com>
>> 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
>