On 3/14/16, Mark Dilger <hornschnor...@gmail.com> wrote: > The first thing I notice about this patch is that > src/include/datatype/timestamp.h > has some #defines that are brittle. The #defines have comments explaining > their logic, but I'd rather embed that in the #define directly: > > This: > > +#ifdef HAVE_INT64_TIMESTAMP > +#define MIN_TIMESTAMP INT64CONST(-211813488000000000) > +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC */ > +#define MAX_TIMESTAMP INT64CONST(9223371331200000000) > +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC > */ > +#else > +#define MIN_TIMESTAMP -211813488000.0 > +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 */ > +#define MAX_TIMESTAMP 9223371331200.0 > +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 */ > +#endif > > Could be written as: > > #ifdef HAVE_INT64_TIMESTAMP > #define MIN_TIMESTAMP > ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY) > #define MAX_TIMESTAMP > ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY) > #else > #define MIN_TIMESTAMP > ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY) > #define MAX_TIMESTAMP > ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY) > #endif > > I assume modern compilers would convert these to the same constants at > compile-time,
Firstly, Postgres is compiling not only by modern compilers. > rather than impose a runtime penalty. Secondary, It is hard to write it correctly obeying Postgres' coding standard (e.g. 80-columns border) and making it clear: it is not so visual difference between USECS_PER_DAY and SECS_PER_DAY in the expressions above (but it is vivid in comments in the file). Also a questions raise "Why is INT64CONST(0) necessary in `#else' block" (whereas `float' is necessary there) or "Why is INT64CONST set for MIN_TIMESTAMP, but not for MAX_TIMESTAMP?" (JULIAN_MAX4STAMPS is _not_ int64). The file is full of constants. For example, JULIAN_MAX and SECS_PER_DAY are one of them. I would leave it as is. > The #defines would be less brittle in > the event, for example, that the postgres epoch were ever changed. I don't think it is real, and even in such case all constants are collected together in the file and will be found and changed at once. > Mark Dilger -- Best regards, Vitaly Burovoy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
