Bruno Haible wrote:
> Hi Bruce,
>
> Got some time to look at the parse-duration module again. I would propose
> these
> changes:
> - Comments, for maintainability.
Thank you. Comments to comments below. :)
> - 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.
Makes sense. Zap it. :)
> 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
*** 5 years is always 1825 days, 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;
> }
>
>
