patch for bug 443 (ENCODE-UNIVERSAL-TIME effectively ignores TIME-ZONE argument)
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. 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. I can't sign in to the bug tracker as the OpenID button doesn't work and I don't remember my common-lisp.net username/password and I don't see a way to get a password reminder on common-lisp.net. A comment on the bug tracker mentions Java 8. If I remember correctly, I tried the previous version of the code with both Java 7 and Java 8 and in both cases it fails. In any event the error seems to be pretty clearly a mistake in ENCODE-UNIVERSAL-TIME so I would be surprised if different Java versions yielded different results. Hope this helps, and all the best. Robert Dodier PS. This patch was taken against r14981 on armedbear-svn/trunk.
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.
Nice catch of the wrong use of units in GET-TIME-ZONE. This provides more evidence that the code [implementing "time of the time" daylight savings semantics][r14840] should never have been committed in the first place, especially given that even after applying [r14995][] we still have failures in the ANSI test suite with DECODE-UNIVERSAL-TIME.3 DECODE-UNIVERSAL-TIME.4 DECODE-UNIVERSAL-TIME.5 ENCODE-UNIVERSAL-TIME.1 ENCODE-UNIVERSAL-TIME.3 that were introduced with [r14840][]. Frankly, I don't completely understand all the things that time.lisp is trying to do with time interpretation, so part of the fault lies with me. I am in the process of incrementally defining my own tests for {DECODE,ENCODE}-UNIVERSAL-TIME.* to understand the code a bit better, as I find it a bit easier than trying to understand actually what the ANSI-TEST suite is trying to do. [r14840]: http://abcl.org/trac/changeset/14840
I can't sign in to the bug tracker as the OpenID button doesn't work and I don't remember my common-lisp.net username/password and I don't see a way to get a password reminder on common-lisp.net.
Yes, OpenID on the ABCL Trac does not currently work, as Google stop serving OAuth in favor of Oauth2 to which an upgrade was not possible on our side the last time I looked at it (18 months ago?) And there is unfortunately no self-service administration for the common-lisp.net HTTP authentication credentials. If you wish to get a reset, please email me privately with your username, and I'll reset it for you. […]
Hope this helps, and all the best.
It helps tremendously with another blocker on the road to the imminent-for-real-now [abcl-1.5.0][] [abcl-1.5.0]: http://abcl.org/trac/milestone/1.5.0 -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
On Sat, Apr 15, 2017 at 10:35 PM, Mark Evenson <evenson@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. -- Scott
On 4/17/17 02:03, Scott L. Burson wrote:
On Sat, Apr 15, 2017 at 10:35 PM, Mark Evenson <evenson@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.)
[…] My apologies: it seems that I am indeed largely at fault for misapplying and misunderstanding Scott's original patch. Thanks for the more detailed explanation. I'll get some coffee, and attempt re-examine the problem with wider eyes. -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
After reapplying the original patch (and thereby removing Robert Dodier's changes), things seem much better in that the implementation now no longer ignores the time zone argument to ENCODE-UNIVERSAL-TIME and the ANSI test regressions have disappeared. I consider this issue to be resolved unless someone else pipes up. -- "A screaming comes across the sky. It has happened before, but there is nothing to compare to it now."
participants (3)
-
Mark Evenson -
Robert Dodier -
Scott L. Burson