Merge lp:~epics-core/epics-base/add-epicstime-from-gmtm into lp:~epics-core/epics-base/3.14

[Ralph Lange]

Adds the missing conversion from GMTM (struct tm without timezone) to epicsTime.

Also adds a simple round trip conversion test (same as done for the conversions to the other time types).

[Ralph Lange (ralph-lange)]

Matching documentation change in
https://code.launchpad.net/~epics-documenters/epics-appdev/add-epicstime-from-gmtm

[Andrew Johnson (anj)]

Looks good & works for me; merging with minor tweaks.

review: Approve

[Ralph Lange (ralph-lange)]

Well,

The nSec overflow is actually being handled in the epicsTime(sec, nSec) constructor - that's why I was removing the code in the conversion constructors and calling the former. After your tweak we now have the functionally identical overflow handling code in three places (in a single source file). Intentionally?

[Andrew Johnson (anj)]

The double construction and assignment to *this (first 4 new lines of your patch, also repeated lower down) triggered a code smell response in my brain. BTW I rewrote your seconds_in_day calculation only after doing some comparisons using https://gcc.godbolt.org/ -- a very useful site when doing code optimization.

The epicsTime(sec, nSec) constructor currently always does the division and remainder operations (I'm fixing that), and while I wanted to avoid those when they weren't necessary, I didn't want to complicate the code by calling different constructors depending on whether there's an overflow or not.

However I just realized that both of our code versions are broken on systems where time_t is not an integer, which we take great pains to handle properly elsewhere in epicsTime.cpp and in the previous version of this code. If time_t can hold fractional seconds then the code's assumption that the epicsTime(ansiTimeTicks) constructor always results in the nSec field being zero is wrong.

The best solution seems to be to remove my overflow checks and call this->addNanoSec(tm.nSec), as is done by some other converter-constructors. For performance on systems with only soft-float I have just rewritten the addNanoSec() method to avoid the cast to double and to not use operator +.

[Ralph Lange (ralph-lange)]

The double construction and assignment to *this were part of the original code.

I was quite suspicious, did also notice the smell, did some reading, and in the end decided that in this specific class this unusual pattern was eventually harmless. (The original might have been safe for non-integer time_t, which I certainly did not recognize and screwed up.)

Your soft-float optimized changes definitely sound useful.

=== modified file 'src/libCom/osi/epicsTime.cpp'
--- src/libCom/osi/epicsTime.cpp	2014-01-29 22:52:22 +0000
+++ src/libCom/osi/epicsTime.cpp	2016-10-04 14:45:07 +0000
@@ -287,11 +287,80 @@
         throwWithLocation ( formatProblemWithStructTM () );
     }
 
-    *this = epicsTime (ansiTimeTicks);
-
-    unsigned long nSecAdj = tm.nSec % nSecPerSec;
-    unsigned long secAdj = tm.nSec / nSecPerSec;
-    *this = epicsTime ( this->secPastEpoch+secAdj, this->nSec+nSecAdj );
+    // Do the epoch conversion
+    *this = epicsTime (ansiTimeTicks);
+    // Add the nSec part
+    *this = epicsTime (this->secPastEpoch, tm.nSec);
+}
+
+//
+// epicsTime (const gm_tm_nano_sec &tm)
+//
+
+// do conversion avoiding the timezone mechanism
+static inline epicsInt32 is_leap(epicsInt32 year)
+{
+    if (year % 400 == 0)
+        return 1;
+    if (year % 100 == 0)
+        return 0;
+    if (year % 4 == 0)
+        return 1;
+    return 0;
+}
+
+static inline epicsInt32 days_from_0(epicsInt32 year)
+{
+    year--;
+    return 365 * year + (year / 400) - (year / 100) + (year / 4);
+}
+
+static inline epicsInt32 days_from_1970(epicsInt32 year)
+{
+    static const int days_from_0_to_1970 = days_from_0(1970);
+    return days_from_0(year) - days_from_0_to_1970;
+}
+
+static inline epicsInt32 days_from_1jan(epicsInt32 year,
+                                        epicsInt32 month,
+                                        epicsInt32 day)
+{
+    static const epicsInt32 days[2][12] =
+    {
+        { 0,31,59,90,120,151,181,212,243,273,304,334},
+        { 0,31,60,91,121,152,182,213,244,274,305,335}
+    };
+    return days[is_leap(year)][month-1] + day - 1;
+}
+
+epicsTime::epicsTime (const gm_tm_nano_sec &tm)
+{
+    time_t_wrapper ansiTimeTicks;
+    int year = tm.ansi_tm.tm_year + 1900;
+    int month = tm.ansi_tm.tm_mon;
+    if (month > 11) {
+        year += month / 12;
+        month %= 12;
+    } else if (month < 0) {
+        int years_diff = (-month + 11) / 12;
+        year  -= years_diff;
+        month += 12 * years_diff;
+    }
+    month++;
+    int day = tm.ansi_tm.tm_mday;
+    int day_of_year = days_from_1jan(year, month, day);
+    int days_since_epoch = days_from_1970(year) + day_of_year;
+
+    const time_t seconds_in_day = 3600 * 24;
+    ansiTimeTicks.ts = seconds_in_day * days_since_epoch
+            + 3600 * tm.ansi_tm.tm_hour
+            + 60 * tm.ansi_tm.tm_min
+            + tm.ansi_tm.tm_sec;
+
+    // Do the epoch conversion
+    *this = epicsTime (ansiTimeTicks);
+    // Add the nSec part
+    *this = epicsTime (this->secPastEpoch, tm.nSec);
 }
 
 //
@@ -905,6 +974,19 @@
         }
         return epicsTimeOK;
     }
+    epicsShareFunc int epicsShareAPI epicsTimeFromGMTM (epicsTimeStamp *pDest, const struct tm *pSrc, unsigned long nSecSrc)
+    {
+        try {
+            gm_tm_nano_sec tmns;
+            tmns.ansi_tm = *pSrc;
+            tmns.nSec = nSecSrc;
+            *pDest = epicsTime (tmns);
+        }
+        catch (...) {
+            return epicsTimeERROR;
+        }
+        return epicsTimeOK;
+    }
     epicsShareFunc int epicsShareAPI epicsTimeToTimespec (struct timespec *pDest, const epicsTimeStamp *pSrc)
     {
         try {
@@ -1036,4 +1118,3 @@
         }
     }
 }
-

=== modified file 'src/libCom/osi/epicsTime.h'
--- src/libCom/osi/epicsTime.h	2009-01-28 20:01:41 +0000
+++ src/libCom/osi/epicsTime.h	2016-10-04 14:45:07 +0000
@@ -105,10 +105,12 @@
     epicsTime & operator = ( const local_tm_nano_sec & );
 
     /*
-     * convert to ANSI Cs "struct tm" (with nano seconds)
+     * convert to and from ANSI Cs "struct tm" (with nano seconds)
      * adjusted for GM time (UTC)
      */
     operator gm_tm_nano_sec () const;
+    epicsTime ( const gm_tm_nano_sec & );
+    epicsTime & operator = ( const gm_tm_nano_sec & );
 
     /* convert to and from POSIX RTs "struct timespec" */
     operator struct timespec () const;
@@ -194,13 +196,15 @@
 epicsShareFunc int epicsShareAPI epicsTimeFromTime_t (
     epicsTimeStamp * pDest, time_t src );
 
-/*convert to and from ANSI C's "struct tm" with nano seconds */
+/* convert to and from ANSI C's "struct tm" with nano seconds */
 epicsShareFunc int epicsShareAPI epicsTimeToTM (
     struct tm * pDest, unsigned long * pNSecDest, const epicsTimeStamp * pSrc );
 epicsShareFunc int epicsShareAPI epicsTimeToGMTM (
     struct tm * pDest, unsigned long * pNSecDest, const epicsTimeStamp * pSrc );
 epicsShareFunc int epicsShareAPI epicsTimeFromTM (
     epicsTimeStamp * pDest, const struct tm * pSrc, unsigned long nSecSrc );
+epicsShareFunc int epicsShareAPI epicsTimeFromGMTM (
+    epicsTimeStamp * pDest, const struct tm * pSrc, unsigned long nSecSrc );
 
 /* convert to and from POSIX RT's "struct timespec" */
 epicsShareFunc int epicsShareAPI epicsTimeToTimespec (
@@ -312,6 +316,11 @@
     return *this = epicsTime ( rhs );
 }
 
+inline epicsTime & epicsTime::operator = ( const gm_tm_nano_sec & rhs )
+{
+    return *this = epicsTime ( rhs );
+}
+
 inline epicsTime & epicsTime::operator = ( const struct timespec & rhs )
 {
     *this = epicsTime ( rhs );

=== modified file 'src/libCom/test/epicsTimeTest.cpp'
--- src/libCom/test/epicsTimeTest.cpp	2014-01-29 22:52:22 +0000
+++ src/libCom/test/epicsTimeTest.cpp	2016-10-04 14:45:07 +0000
@@ -48,7 +48,7 @@
     const int wasteTime = 100000;
     const int nTimes = 10;
 
-    testPlan(15 + nTimes * 18);
+    testPlan(15 + nTimes * 19);
 
     try {
         const epicsTimeStamp epochTS = {0, 0};
@@ -205,6 +205,11 @@
         epicsTime beginANSI = ansiDate;
         testOk1(beginANSI + diff == now);
 
+        // test struct gmtm round-trip conversion
+        gm_tm_nano_sec ansiGmDate = begin;
+        epicsTime beginGMANSI = ansiGmDate;
+        testOk1(beginGMANSI + diff == now);
+
         // test struct timespec round-trip conversion
         struct timespec ts = begin;
         epicsTime beginTS = ts;