bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: parse-duration tweaks


From: Bruce Korb
Subject: Re: parse-duration tweaks
Date: Tue, 16 Dec 2008 10:21:47 -0800
User-agent: Thunderbird 2.0.0.18 (X11/20081112)

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  <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
***  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;
>  }
>  
> 





reply via email to

[Prev in Thread] Current Thread [Next in Thread]