Thread: Question about src/timezone/zic.c
I've recently found some time on my hands and have decided to see about contributing to the PostgreSQL project, so I've started browsing the code to get somewhat familiar with it.
The file src/timezone/zic.c caused me to raise my eyebrows a bit and I have a question to ask because of this.
I first noticed some loops dealing with the time that I suspect could be replaced with straight line code calling the julian date conversions. But in attempting to understand exactly what the loops were doing, I saw some calls to eitol() which is the function that I'm really questioning. The code for eitol() is as follows:
static long
eitol(int i)
{
long l;
l = i;
if ((i < 0 && l >= 0) || (i == 0 && l != 0) || (i > 0 && l <= 0))
{
(void) fprintf(stderr,
_("%s: %d did not sign extend correctly\n"),
progname, i);
exit(EXIT_FAILURE);
}
return l;
}
As you can see, all that function does is perform a length extension of a signed int to a signed long. Additionally, it has a fair amount of code to verify that the sign extension was properly done. Since the sign extension is actually performed via the simple "l=i;" statement towards the beginning of the function, it's rather obvious that we're relying upon the compiler to perform the sign extension. Additionally, since this function doesn't have any recovery if an invalid extension is performed, it means that if a faulty compiler is encountered, PostgreSQL will always fail to operate the instant any timezone computation is performed.
This raises the question of "Does anyone on this mailing list know of any C compiler that fails to properly sign extend an assignment of an int to a long?" Or to put it another way, "Are there any C compilers that fail to properly perform integer promotion from int to long?"
As things stand, it looks to me like that function eitol() can be simply deleted and the 22 calls to that function also removed. Shorter, simpler,faster code is always a good thing after all.
Thank you for reading,
John Cochran
John Cochran <j69cochran@gmail.com> writes:
> [ useless code in zic.c ]
I agree with you that that function looks pretty pointless, but here's the
thing: that's not our code. Most of the code in src/timezone is taken
directly from the IANA tzcode distribution (see src/timezone/README), and
we're not very interested in diverging further from that upstream than
we have to. Doing so would just complicate merging future upstream fixes.
So if you want to pursue the question of whether eitol is worth its
weight, the right thing to do would be to ask about it on the upstream
mailing list (looks like tz at iana.org is the current list name).
One thing that *would* be worth doing is merging upstream's recent
updates; for all I know they've already dealt with this. According
to the README, we last synced with the 2010c tzcode release, so we're
short a few years worth of upstream bug fixes there.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> One thing that *would* be worth doing is merging upstream's recent
> updates; for all I know they've already dealt with this. According
> to the README, we last synced with the 2010c tzcode release, so we're
> short a few years worth of upstream bug fixes there.
Agreed- but just to level-set expectations here, don't expect it to be a
trivial or easily done thing. I looked into a bit when hitting a few
Coverity complaints (which appear to be innocuous based on both my
review and that of others who have commented on it) and was quite
surprised at how far the code has diverged. That code appears to be
quite actively updated and we may want to try and work out a way to keep
up better than the occational "whenever someone feels up to it and has
the time to" updates that we're doing now.
My thought would be to try and make this part of the major version
release process somehow, perhaps explicitly include it as a 'patch' or
task in the last CF before we feature freeze each year.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> One thing that *would* be worth doing is merging upstream's recent
>> updates; for all I know they've already dealt with this. According
>> to the README, we last synced with the 2010c tzcode release, so we're
>> short a few years worth of upstream bug fixes there.
> Agreed- but just to level-set expectations here, don't expect it to be a
> trivial or easily done thing. I looked into a bit when hitting a few
> Coverity complaints (which appear to be innocuous based on both my
> review and that of others who have commented on it) and was quite
> surprised at how far the code has diverged. That code appears to be
> quite actively updated and we may want to try and work out a way to keep
> up better than the occational "whenever someone feels up to it and has
> the time to" updates that we're doing now.
Yeah. IIRC, the existing divergences come mostly from (1) running
pgindent on the zic code, which we could certainly refrain from; and
(2) modifying their code to suppress assorted compiler warnings, for
example they were still not using ANSI C function definition syntax
last I looked. I would be loath to give up (2), so unless upstream
has modernized their code it might be hard to eliminate all the
divergences.
If it weren't for the differing philosophies about compiler warnings,
I'd wish we could create a subdirectory that was *exactly* their
tzcode distribution, in much the same way that the data/ subdirectory
is exactly their tzdata distribution. Then tracking their code updates
would be relatively mindless.
regards, tom lane