Re: Macros for time magic values - Mailing list pgsql-hackers

Applied.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> It has bothered me that many of our time routines use special magic
> constants for time values, e.g. 24, 12, 60, etc.
> 
> The attached patch changes these magic constants to macros to clarify
> the code.  I would like to apply this for 9.1 as a cleanup.
> 
> -- 
>   Bruce Momjian  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   + It's impossible for everything to be true. +

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

> diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
> index 80dd10b..f96fa6c 100644
> --- a/src/backend/utils/adt/date.c
> +++ b/src/backend/utils/adt/date.c
> @@ -2612,7 +2612,7 @@ timetz_zone(PG_FUNCTION_ARGS)
>      type = DecodeSpecial(0, lowzone, &val);
>  
>      if (type == TZ || type == DTZ)
> -        tz = val * 60;
> +        tz = val * MINS_PER_HOUR;
>      else
>      {
>          tzp = pg_tzset(tzname);
> diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
> index 85f0206..f0fe2e3 100644
> --- a/src/backend/utils/adt/datetime.c
> +++ b/src/backend/utils/adt/datetime.c
> @@ -342,7 +342,7 @@ j2date(int jd, int *year, int *month, int *day)
>      *year = y - 4800;
>      quad = julian * 2141 / 65536;
>      *day = julian - 7834 * quad / 256;
> -    *month = (quad + 10) % 12 + 1;
> +    *month = (quad + 10) % MONTHS_PER_YEAR + 1;
>  
>      return;
>  }    /* j2date() */
> @@ -952,8 +952,8 @@ DecodeDateTime(char **field, int *ftype, int nf,
>                   * DecodeTime()
>                   */
>                  /* test for > 24:00:00 */
> -                if (tm->tm_hour > 24 ||
> -                    (tm->tm_hour == 24 &&
> +                if (tm->tm_hour > HOURS_PER_DAY ||
> +                    (tm->tm_hour == HOURS_PER_DAY &&
>                       (tm->tm_min > 0 || tm->tm_sec > 0 || *fsec > 0)))
>                      return DTERR_FIELD_OVERFLOW;
>                  break;
> @@ -1371,12 +1371,12 @@ DecodeDateTime(char **field, int *ftype, int nf,
>          return dterr;
>  
>      /* handle AM/PM */
> -    if (mer != HR24 && tm->tm_hour > 12)
> +    if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
>          return DTERR_FIELD_OVERFLOW;
> -    if (mer == AM && tm->tm_hour == 12)
> +    if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
>          tm->tm_hour = 0;
> -    else if (mer == PM && tm->tm_hour != 12)
> -        tm->tm_hour += 12;
> +    else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
> +        tm->tm_hour += HOURS_PER_DAY / 2;
>  
>      /* do additional checking for full date specs... */
>      if (*dtype == DTK_DATE)
> @@ -2058,17 +2058,18 @@ DecodeTimeOnly(char **field, int *ftype, int nf,
>          return dterr;
>  
>      /* handle AM/PM */
> -    if (mer != HR24 && tm->tm_hour > 12)
> +    if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
>          return DTERR_FIELD_OVERFLOW;
> -    if (mer == AM && tm->tm_hour == 12)
> +    if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
>          tm->tm_hour = 0;
> -    else if (mer == PM && tm->tm_hour != 12)
> -        tm->tm_hour += 12;
> +    else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
> +        tm->tm_hour += HOURS_PER_DAY / 2;
>  
> -    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60 || tm->tm_hour > 24 ||
> +    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
> +        tm->tm_hour > HOURS_PER_DAY ||
>      /* test for > 24:00:00 */
> -        (tm->tm_hour == 24 &&
> +        (tm->tm_hour == HOURS_PER_DAY &&
>           (tm->tm_min > 0 || tm->tm_sec > 0 || *fsec > 0)) ||
>  #ifdef HAVE_INT64_TIMESTAMP
>          *fsec < INT64CONST(0) || *fsec > USECS_PER_SEC
> @@ -2396,13 +2397,15 @@ DecodeTime(char *str, int fmask, int range,
>  
>      /* do a sanity check */
>  #ifdef HAVE_INT64_TIMESTAMP
> -    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60 || *fsec < INT64CONST(0) ||
> +    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR -1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
> +        *fsec < INT64CONST(0) ||
>          *fsec > USECS_PER_SEC)
>          return DTERR_FIELD_OVERFLOW;
>  #else
> -    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60 || *fsec < 0 || *fsec > 1)
> +    if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE ||
> +        *fsec < 0 || *fsec > 1)
>          return DTERR_FIELD_OVERFLOW;
>  #endif
>  
> @@ -2748,9 +2751,9 @@ DecodeTimezone(char *str, int *tzp)
>  
>      if (hr < 0 || hr > 14)
>          return DTERR_TZDISP_OVERFLOW;
> -    if (min < 0 || min >= 60)
> +    if (min < 0 || min >= MINS_PER_HOUR)
>          return DTERR_TZDISP_OVERFLOW;
> -    if (sec < 0 || sec >= 60)
> +    if (sec < 0 || sec >= SECS_PER_MINUTE)
>          return DTERR_TZDISP_OVERFLOW;
>  
>      tz = (hr * MINS_PER_HOUR + min) * SECS_PER_MINUTE + sec;
> @@ -3324,7 +3327,7 @@ DecodeISO8601Interval(char *str,
>              {
>                  case 'Y':
>                      tm->tm_year += val;
> -                    tm->tm_mon += (fval * 12);
> +                    tm->tm_mon += (fval * MONTHS_PER_YEAR);
>                      break;
>                  case 'M':
>                      tm->tm_mon += val;
> @@ -3359,7 +3362,7 @@ DecodeISO8601Interval(char *str,
>                          return DTERR_BAD_FORMAT;
>  
>                      tm->tm_year += val;
> -                    tm->tm_mon += (fval * 12);
> +                    tm->tm_mon += (fval * MONTHS_PER_YEAR);
>                      if (unit == '\0')
>                          return 0;
>                      if (unit == 'T')
> @@ -4155,7 +4158,7 @@ InstallTimeZoneAbbrevs(tzEntry *abbrevs, int n)
>      {
>          strncpy(newtbl[i].token, abbrevs[i].abbrev, TOKMAXLEN);
>          newtbl[i].type = abbrevs[i].is_dst ? DTZ : TZ;
> -        TOVAL(&newtbl[i], abbrevs[i].offset / 60);
> +        TOVAL(&newtbl[i], abbrevs[i].offset / MINS_PER_HOUR);
>      }
>  
>      /* Check the ordering, if testing */
> diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
> index f90d36d..aba1145 100644
> --- a/src/backend/utils/adt/formatting.c
> +++ b/src/backend/utils/adt/formatting.c
> @@ -2129,7 +2129,7 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
>                   * intervals
>                   */
>                  sprintf(s, "%0*d", S_FM(n->suffix) ? 0 : 2,
> -                        tm->tm_hour % (HOURS_PER_DAY / 2) == 0 ? 12 :
> +                        tm->tm_hour % (HOURS_PER_DAY / 2) == 0 ? HOURS_PER_DAY / 2 :
>                          tm->tm_hour % (HOURS_PER_DAY / 2));
>                  if (S_THth(n->suffix))
>                      str_numth(s, s, S_TH_TYPE(n->suffix));
> @@ -2486,14 +2486,14 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
>                  if (!tm->tm_mon)
>                      break;
>                  sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
> -                        rm_months_upper[12 - tm->tm_mon]);
> +                        rm_months_upper[MONTHS_PER_YEAR - tm->tm_mon]);
>                  s += strlen(s);
>                  break;
>              case DCH_rm:
>                  if (!tm->tm_mon)
>                      break;
>                  sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
> -                        rm_months_lower[12 - tm->tm_mon]);
> +                        rm_months_lower[MONTHS_PER_YEAR - tm->tm_mon]);
>                  s += strlen(s);
>                  break;
>              case DCH_W:
> @@ -2779,12 +2779,12 @@ DCH_from_char(FormatNode *node, char *in, TmFromChar *out)
>              case DCH_RM:
>                  from_char_seq_search(&value, &s, rm_months_upper,
>                                       ALL_UPPER, MAX_RM_LEN, n);
> -                from_char_set_int(&out->mm, 12 - value, n);
> +                from_char_set_int(&out->mm, MONTHS_PER_YEAR - value, n);
>                  break;
>              case DCH_rm:
>                  from_char_seq_search(&value, &s, rm_months_lower,
>                                       ALL_LOWER, MAX_RM_LEN, n);
> -                from_char_set_int(&out->mm, 12 - value, n);
> +                from_char_set_int(&out->mm, MONTHS_PER_YEAR - value, n);
>                  break;
>              case DCH_W:
>                  from_char_parse_int(&out->w, &s, n);
> @@ -3236,16 +3236,16 @@ do_to_timestamp(text *date_txt, text *fmt,
>  
>      if (tmfc.clock == CLOCK_12_HOUR)
>      {
> -        if (tm->tm_hour < 1 || tm->tm_hour > 12)
> +        if (tm->tm_hour < 1 || tm->tm_hour > HOURS_PER_DAY / 2)
>              ereport(ERROR,
>                      (errcode(ERRCODE_INVALID_DATETIME_FORMAT),
>                       errmsg("hour \"%d\" is invalid for the 12-hour clock",
>                              tm->tm_hour),
>                       errhint("Use the 24-hour clock, or give an hour between 1 and 12.")));
>  
> -        if (tmfc.pm && tm->tm_hour < 12)
> -            tm->tm_hour += 12;
> -        else if (!tmfc.pm && tm->tm_hour == 12)
> +        if (tmfc.pm && tm->tm_hour < HOURS_PER_DAY / 2)
> +            tm->tm_hour += HOURS_PER_DAY / 2;
> +        else if (!tmfc.pm && tm->tm_hour == HOURS_PER_DAY / 2)
>              tm->tm_hour = 0;
>      }
>  
> @@ -3347,7 +3347,7 @@ do_to_timestamp(text *date_txt, text *fmt,
>  
>              y = ysum[isleap(tm->tm_year)];
>  
> -            for (i = 1; i <= 12; i++)
> +            for (i = 1; i <= MONTHS_PER_YEAR; i++)
>              {
>                  if (tmfc.ddd < y[i])
>                      break;
> diff --git a/src/backend/utils/adt/nabstime.c b/src/backend/utils/adt/nabstime.c
> index 0e25c5f..4b61b21 100644
> --- a/src/backend/utils/adt/nabstime.c
> +++ b/src/backend/utils/adt/nabstime.c
> @@ -179,13 +179,13 @@ tm2abstime(struct pg_tm * tm, int tz)
>  
>      /* validate, before going out of range on some members */
>      if (tm->tm_year < 1901 || tm->tm_year > 2038 ||
> -        tm->tm_mon < 1 || tm->tm_mon > 12 ||
> +        tm->tm_mon < 1 || tm->tm_mon > MONTHS_PER_YEAR ||
>          tm->tm_mday < 1 || tm->tm_mday > 31 ||
>          tm->tm_hour < 0 ||
> -        tm->tm_hour > 24 ||        /* test for > 24:00:00 */
> -        (tm->tm_hour == 24 && (tm->tm_min > 0 || tm->tm_sec > 0)) ||
> -        tm->tm_min < 0 || tm->tm_min > 59 ||
> -        tm->tm_sec < 0 || tm->tm_sec > 60)
> +        tm->tm_hour > HOURS_PER_DAY ||        /* test for > 24:00:00 */
> +        (tm->tm_hour == HOURS_PER_DAY && (tm->tm_min > 0 || tm->tm_sec > 0)) ||
> +        tm->tm_min < 0 || tm->tm_min > MINS_PER_HOUR - 1 ||
> +        tm->tm_sec < 0 || tm->tm_sec > SECS_PER_MINUTE)
>          return INVALID_ABSTIME;
>  
>      day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - UNIX_EPOCH_JDATE;
> diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
> index 1c2c160..45e7002 100644
> --- a/src/backend/utils/adt/timestamp.c
> +++ b/src/backend/utils/adt/timestamp.c
> @@ -4422,7 +4422,7 @@ timestamp_zone(PG_FUNCTION_ARGS)
>  
>      if (type == TZ || type == DTZ)
>      {
> -        tz = -(val * 60);
> +        tz = -(val * MINS_PER_HOUR);
>          result = dt2local(timestamp, tz);
>      }
>      else
> @@ -4596,7 +4596,7 @@ timestamptz_zone(PG_FUNCTION_ARGS)
>  
>      if (type == TZ || type == DTZ)
>      {
> -        tz = val * 60;
> +        tz = val * MINS_PER_HOUR;
>          result = dt2local(timestamp, tz);
>      }
>      else
> diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
> index 9e51b58..e14285f 100644
> --- a/src/include/utils/timestamp.h
> +++ b/src/include/utils/timestamp.h
> @@ -81,6 +81,8 @@ typedef struct
>   */
>  #define DAYS_PER_MONTH    30        /* assumes exactly 30 days per month */
>  #define HOURS_PER_DAY    24        /* assume no daylight savings time changes */
> +#define MINS_PER_HOUR    60        /* assume no daylight savings time changes */
> +#define SECS_PER_MINUTE    60        /* assume no daylight savings time changes */
>  
>  /*
>   *    This doesn't adjust for uneven daylight savings time intervals or leap

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +