patch for bug 443 (ENCODE-UNIVERSAL-TIME effectively ignores TIME-ZONE argument)
Scott L. Burson
Scott at sympoiesis.com
Mon Apr 17 00:03:36 UTC 2017
On Sat, Apr 15, 2017 at 10:35 PM, Mark Evenson <evenson at panix.com> wrote:
> On 4/16/17 01:08, Robert Dodier wrote:
> > Hi, here is a patch to fix bug 443. With this change, Maxima + ABCL
> > passes its tests (in tests/rtest11.mac for the record) for
> > parse_timedate which calls ENCODE-UNIVERSAL-TIME.
> Thanks for the patch; it has been applied as [r14995].
> [r14995]: http://abcl.org/trac/changeset/14995
> > Incidentally this patch also fixes an unreported bug in
> > DECODE-UNIVERSAL-TIME: the daylight saving flag is reported
> > incorrectly because EXT:GET-TIME-ZONE expects its argument in
> > milliseconds, not seconds.
The patch I submitted did not have either of these bugs. The
ENCODE-UNIVERSAL-TIME bug was a merge error: the original had two very
similar lines, in the first and third COND clause, and the hunk was
intended for the third clause but was applied to the first. A close
look at the emailed patch will confirm this.
And then you intentionally changed GET-TIME-ZONE to take a Unix time
in milliseconds rather than a universal time, and didn't fix the call
sites. (I don't agree with the change in interface; I think CL
functions should use universal times whenever possible.)
In fairness, you did ask people to test the patch after you merged it,
and obviously I never did. (I'm still using 1.3.2 with my patches; I
haven't tried 1.4.0.) Nor have I bothered to run the ANSI test suite.
There are a couple of problems with Robert's patch. First,
multiplying the universal time by 1000 isn't sufficient; you need to
adjust for the difference in epochs (1900-1-1 vs. 1970-1-1) first, by
subtracting 2208988800. This probably explains why some of the ANSI
tests are still failing. Second, by reordering the first two COND
clauses, it breaks the case where TIME-ZONE is supplied but the year
is after 2037.
(This raises the question of why there's special handling for
post-2037 at all. It's not clear to me. I suspect this is a leftover
from a time that the code was calling some Unix library function to
help it with the conversion, but I don't see it doing that now, so I
think this clause can probably be removed.)
My recommendation is to put things the way I suggested, with
GET-TIME-ZONE taking a universal time and a Java function
'getTimeZone' (or whatever) taking a Unix time in milliseconds, and
applying my patch correctly to the pre-14840 version of
ENCODE-UNIVERSAL-TIME. I've been using the code this way routinely
since I submitted the patch, and haven't noticed any further bugs.
More information about the armedbear-devel