parse-duration tweaks

[Bruno Haible]

Hi Bruce,

Got some time to look at the parse-duration module again. I would propose these
changes:
  - Comments, for maintainability.
  - Don't put side effects into function call arguments. These functions may
    be implemented as macros on inferior platforms, you never know.
  - Don't test errno if  res != BAD_TIME. For example, the free() call at the
    end of parse_period() may set errno.
  - Save errno around the call to fprintf. fprintf may set errno.

Also, can you clarify the purpose of having the
  fprintf (stderr, _("Invalid time duration:  %s\n"), pz);
in the function? Usually, when a function is designed as a library
function, it provides an error indicator through the return value (which
parse_duration does) and leaves the responsibility of signalling errors
to the caller.


2008-12-16  Bruno Haible  <address@hidden>

        * lib/parse-duration.h (parse_duration): Document return value
        convention.
        * lib/parse-duration.c: Include specification header first. Add
        comments.
        (parse_year_month_day, parse_hour_minute_second): Move side effects
        outside of strchr call.
        (parse_non_iso8601): Move side effects outside of isspace call.
        (parse_duration): Don't test errno is res != BAD_TIME. Save errno
        around fprintf call.

--- lib/parse-duration.h.orig   2008-12-16 12:21:06.000000000 +0100
+++ lib/parse-duration.h        2008-12-16 11:25:07.000000000 +0100
@@ -47,9 +47,11 @@
     yy Y mm M ww W dd D
 
   or it may be empty and followed by a 'T'.  The "yyyymmdd" must be eight
-  digits long.  Note:  months are always 30 days and years are always 365
-  days long.  5 years is always 1825, not 1826 or 1827 depending on leap
-  year considerations.  3 months is always 90 days.  There is no consideration
+  digits long.
+
+  NOTE!  Months are always 30 days and years are always 365 days long.
+  5 years is always 1825, not 1826 or 1827 depending on leap year
+  considerations.  3 months is always 90 days.  There is no consideration
   for how many days are in the current, next or previous months.
 
   For the final format:
@@ -75,8 +77,12 @@
 
 #include <time.h>
 
+/* Return value when a valid duration cannot be parsed.  */
 #define BAD_TIME        ((time_t)~0)
 
+/* Parses the given string.  If it has the syntax of a valid duration,
+   this duration is returned.  Otherwise, the return value is BAD_TIME,
+   and errno is set to either EINVAL (bad syntax) or ERANGE (out of range).  */
 extern time_t parse_duration(char const * in_pz);
 
 #endif /* GNULIB_PARSE_DURATION_H */
--- lib/parse-duration.c.orig   2008-12-16 12:21:06.000000000 +0100
+++ lib/parse-duration.c        2008-12-16 12:04:25.000000000 +0100
@@ -17,6 +17,9 @@
 
 #include <config.h>
 
+/* Specification.  */
+#include "parse-duration.h"
+
 #include <ctype.h>
 #include <errno.h>
 #include <limits.h>
@@ -25,8 +28,6 @@
 #include <string.h>
 #include "xalloc.h"
 
-#include "parse-duration.h"
-
 #ifndef _
 #define _(_s)  _s
 #endif
@@ -57,18 +58,23 @@
 
 #define TIME_MAX        0x7FFFFFFF
 
+/* Wrapper around strtoul that does not require a cast.  */
 static unsigned long inline
 str_const_to_ul (cch_t * str, cch_t ** ppz, int base)
 {
   return strtoul (str, (char **)ppz, base);
 }
 
+/* Wrapper around strtol that does not require a cast.  */
 static long inline
 str_const_to_l (cch_t * str, cch_t ** ppz, int base)
 {
   return strtol (str, (char **)ppz, base);
 }
 
+/* Returns BASE + VAL * SCALE, interpreting BASE = BAD_TIME
+   with errno set as an error situation, and returning BAD_TIME
+   with errno set in an error situation.  */
 static time_t inline
 scale_n_add (time_t base, time_t val, int scale)
 {
@@ -95,6 +101,7 @@
   return base + val;
 }
 
+/* After a number HH has been parsed, parse subsequent :MM or :MM:SS.  */
 static time_t
 parse_hr_min_sec (time_t start, cch_t * pz)
 {
@@ -118,7 +125,8 @@
     }
 
   /* allow for trailing spaces */
-  while (isspace ((unsigned char)*pz))   pz++;
+  while (isspace ((unsigned char)*pz))
+    pz++;
   if (*pz != NUL)
     {
       errno = EINVAL;
@@ -128,6 +136,9 @@
   return start;
 }
 
+/* Parses a value and returns BASE + value * SCALE, interpreting
+   BASE = BAD_TIME with errno set as an error situation, and returning
+   BAD_TIME with errno set in an error situation.  */
 static time_t
 parse_scaled_value (time_t base, cch_t ** ppz, cch_t * endp, int scale)
 {
@@ -141,17 +152,20 @@
   val = str_const_to_ul (pz, &pz, 10);
   if (errno != 0)
     return BAD_TIME;
-  while (isspace ((unsigned char)*pz))   pz++;
+  while (isspace ((unsigned char)*pz))
+    pz++;
   if (pz != endp)
     {
       errno = EINVAL;
       return BAD_TIME;
     }
 
-  *ppz =  pz;
+  *ppz = pz;
   return scale_n_add (base, val, scale);
 }
 
+/* Parses the syntax YEAR-MONTH-DAY.
+   PS points into the string, after "YEAR", before "-MONTH-DAY".  */
 static time_t
 parse_year_month_day (cch_t * pz, cch_t * ps)
 {
@@ -159,7 +173,8 @@
 
   res = parse_scaled_value (0, &pz, ps, SEC_PER_YEAR);
 
-  ps = strchr (++pz, '-');
+  pz++; /* over the first '-' */
+  ps = strchr (pz, '-');
   if (ps == NULL)
     {
       errno = EINVAL;
@@ -167,11 +182,12 @@
     }
   res = parse_scaled_value (res, &pz, ps, SEC_PER_MONTH);
 
-  pz++;
+  pz++; /* over the second '-' */
   ps = pz + strlen (pz);
   return parse_scaled_value (res, &pz, ps, SEC_PER_DAY);
 }
 
+/* Parses the syntax YYYYMMDD.  */
 static time_t
 parse_yearmonthday (cch_t * in_pz)
 {
@@ -201,6 +217,7 @@
   return parse_scaled_value (res, &pz, buf + 2, SEC_PER_DAY);
 }
 
+/* Parses the syntax yy Y mm M ww W dd D.  */
 static time_t
 parse_YMWD (cch_t * pz)
 {
@@ -233,7 +250,8 @@
       pz++;
     }
 
-  while (isspace ((unsigned char)*pz))   pz++;
+  while (isspace ((unsigned char)*pz))
+    pz++;
   if (*pz != NUL)
     {
       errno = EINVAL;
@@ -243,6 +261,8 @@
   return res;
 }
 
+/* Parses the syntax HH:MM:SS.
+   PS points into the string, after "HH", before ":MM:SS".  */
 static time_t
 parse_hour_minute_second (cch_t * pz, cch_t * ps)
 {
@@ -250,7 +270,8 @@
 
   res = parse_scaled_value (0, &pz, ps, SEC_PER_HR);
 
-  ps = strchr (++pz, ':');
+  pz++;
+  ps = strchr (pz, ':');
   if (ps == NULL)
     {
       errno = EINVAL;
@@ -264,6 +285,7 @@
   return parse_scaled_value (res, &pz, ps, 1);
 }
 
+/* Parses the syntax HHMMSS.  */
 static time_t
 parse_hourminutesecond (cch_t * in_pz)
 {
@@ -293,6 +315,7 @@
   return parse_scaled_value (res, &pz, buf + 2, 1);
 }
 
+/* Parses the syntax hh H mm M ss S.  */
 static time_t
 parse_HMS (cch_t * pz)
 {
@@ -318,7 +341,8 @@
       pz++;
     }
 
-  while (isspace ((unsigned char)*pz))   pz++;
+  while (isspace ((unsigned char)*pz))
+    pz++;
   if (*pz != NUL)
     {
       errno = EINVAL;
@@ -328,6 +352,7 @@
   return res;
 }
 
+/* Parses a time (hours, minutes, seconds) specification in either syntax.  */
 static time_t
 parse_time (cch_t * pz)
 {
@@ -359,16 +384,20 @@
   return res;
 }
 
+/* Returns a substring of the given string, with spaces at the beginning and at
+   the end destructively removed.  */
 static char *
-trim(char * pz)
+trim (char * pz)
 {
   /* trim leading white space */
-  while (isspace ((unsigned char)*pz))  pz++;
+  while (isspace ((unsigned char)*pz))
+    pz++;
 
   /* trim trailing white space */
   {
     char * pe = pz + strlen (pz);
-    while ((pe > pz) && isspace ((unsigned char)pe[-1])) pe--;
+    while ((pe > pz) && isspace ((unsigned char)pe[-1]))
+      pe--;
     *pe = NUL;
   }
 
@@ -462,7 +491,8 @@
       unsigned int mult;
 
       /*  Skip over white space following the number we just parsed. */
-      while (isspace ((unsigned char)*pz))   pz++;
+      while (isspace ((unsigned char)*pz))
+        pz++;
 
       switch (*pz)
         {
@@ -520,7 +550,9 @@
 
       res = scale_n_add (res, val, mult);
 
-      while (isspace ((unsigned char)*++pz))   ;
+      pz++;
+      while (isspace ((unsigned char)*pz))
+        pz++;
       if (*pz == NUL)
         return res;
 
@@ -539,14 +571,16 @@
 parse_duration (char const * pz)
 {
   time_t res = 0;
+  int saved_errno;
 
-  while (isspace ((unsigned char)*pz)) pz++;
+  while (isspace ((unsigned char)*pz))
+    pz++;
 
   do {
     if (*pz == 'P')
       {
         res = parse_period (pz + 1);
-        if ((errno != 0) || (res == BAD_TIME))
+        if (res == BAD_TIME)
           break;
         return res;
       }
@@ -554,7 +588,7 @@
     if (*pz == 'T')
       {
         res = parse_time (pz + 1);
-        if ((errno != 0) || (res == BAD_TIME))
+        if (res == BAD_TIME)
           break;
         return res;
       }
@@ -563,14 +597,14 @@
       break;
 
     res = parse_non_iso8601 (pz);
-    if ((errno == 0) && (res != BAD_TIME))
+    if (res != BAD_TIME)
       return res;
 
   } while (0);
 
+  saved_errno = errno;
   fprintf (stderr, _("Invalid time duration:  %s\n"), pz);
-  if (errno == 0)
-    errno = EINVAL;
+  errno = saved_errno;
   return BAD_TIME;
 }