This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add timegm POSIX call


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).
...

  #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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]