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 <br...@clisp.org>
* 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;
}
