Joseph Koshakow <koshy44@gmail.com> writes:
> Attached is a patch of my first pass. The to_char method isn't finished
> and I need to add a bunch of tests, but everything else is in place. It
> ended up being a fairly large change in case anyone wants to take a look
> the changes so far.
Couple of quick comments:
* You've assumed in a number of places that long == int64. This is wrong
on many platforms. Current project style for use of int64 in printf-type
calls is to cast the argument to (long long) explicitly and use "%lld" (or
"%llu", etc). As for strtoint_64, surely that functionality exists
somewhere already (hopefully with fewer bugs than this has).
* I think that tools like Coverity will complain about how you've
got both calls that check the result of AdjustFractSeconds (etc)
and calls that don't. You might consider coding the latter like
+ /* this can't overflow: */
+ (void) AdjustFractSeconds(fval, itm, fsec, SECS_PER_DAY);
in hopes of (a) silencing those warnings and (b) making the implicit
assumption clear to readers.
* I'm not entirely buying use of pg_time_t, rather than just int64,
in struct itm. I think the point of this struct is to get away
from datetime-specific datatypes. Also, if pg_time_t ever changed
width, it'd create issues for users of this struct. I also note
a number of places in the patch that are assuming that these fields
are int64 not something else.
* I'm a little inclined to rename interval2tm/tm2interval to
interval2itm/itm2interval, as I think it'd be confusing for those
function names to refer to a typedef they no longer use.
* The uses of tm2itm make me a bit itchy. Is that sweeping
upstream-of-there overflow problems under the rug?
* // comments are not project style, either. While pgindent will
convert them to /* ... */ style, the results might not be pleasing.
> One thing I noticed is that interval.c has a ton of code copied and pasted
> from other files. The code seemed out of date from those other files, so
> I tried to bring it up to date and add my changes.
We haven't actually maintained ecpg's copies of backend datatype-specific
code in many years. While bringing that stuff up to speed might be
worthwhile (or perhaps not, given the lack of complaints), I'd see it
as a separate project.
regards, tom lane
