4:45 p.m.
(wasn't in the moderator queue either)
-------- Forwarded Message --------
Subject: Re: [PATCH 21.5] Fix signs in time zone string.
Date: Tue, 30 Sep 2014 12:38:25 +0200
From: Adrian Aichner <adrian.aichner(a)gmail.com>
To: xemacs-patches(a)xemacs.org
Hi Marcus, this patch looks wrong to me.
How could XEmacs and GNU Emacs both be wrong until now?
http://git.savannah.gnu.org/cgit/emacs.git/tree/src/editfns.c#n1937
suggests that they are not including a + character for positive offsets,
which surprised me.
But zone offsets > 0 do not include the - sign, which is the same logic
XEmacs uses before your patch.
Also, why are you removing the REST argument in passing without mention?
Adrian
On 30.09.2014 08:13, Marcus Crestani wrote:
PATCH 21.5
Currently, Fencode_time calculates wrong dates in respect to local
timezones. The problem is caused by switched signs in the code that
builds the time zone string, the attached patch fixes this.
I'll push in two days if nobody objects.
diff --git a/src/ChangeLog b/src/ChangeLog
--- a/src/ChangeLog
+++ b/src/ChangeLog
＠＠ -1,3 +1,9 ＠＠
+2014-09-30 Marcus Crestani <crestani(a)informatik.uni-tuebingen.de>
+
+ * editfns.c (Fencode_time): Switch signs when calculating the time
+ zone string. (The offset indicates the value one must add to the
+ local time to arrive at UTC.)
+
2014-09-23 Jerry James <james(a)xemacs.org>
* floatfns.c (round_two_bignum_1): Fix memory leak.
diff --git a/src/editfns.c b/src/editfns.c
--- a/src/editfns.c
+++ b/src/editfns.c
＠＠ -1155,7 +1155,7 ＠＠
Year numbers less than 100 are treated just like other year numbers.
If you want them to stand for years in this century, you must do that yourself.
-arguments: (SECOND MINUTE HOUR DAY MONTH YEAR &optional ZONE &rest REST)
+arguments: (SECOND MINUTE HOUR DAY MONTH YEAR &optional ZONE)
*/
(int nargs, Lisp_Object *args))
{
＠＠ -1192,7 +1192,7 ＠＠
int abszone = abs (XFIXNUM (zone));
/* #### I have no idea what this conforms to,
but the compiler has stopped whining. */
- sprintf (tzbuf, "XXX%s%d:%02d:%02d", (XFIXNUM (zone) < 0) ?
"-" : "+",
+ sprintf (tzbuf, "XXX%s%d:%02d:%02d", (XFIXNUM (zone) < 0) ?
"+" : "-",
abszone / (60*60), (abszone/60) % 60, abszone % 60);
tzstring = tzbuf;
}
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
6:28 p.m.
>>>>"AA" == Adrian Aichner
<adrian.aichner(a)gmail.com> writes:
AA> How could XEmacs and GNU Emacs both
be wrong until now?
XEmacs wasn't wrong for long, this caused the problem:
changeset: 5762:427a72c6ee17
user: Stephen J. Turnbull <stephen(a)xemacs.org>
date: Sun Sep 15 23:47:37 2013 +0900
summary: Eliminate several compiler (clang) warnings.
diff --git a/src/editfns.c b/src/editfns.c
--- a/src/editfns.c
+++ b/src/editfns.c
＠＠ -1190,7 +1190,9 ＠＠
else if (FIXNUMP (zone))
{
int abszone = abs (XFIXNUM (zone));
- sprintf (tzbuf, "XXX%s%d:%02d:%02d", "-" + (XFIXNUM (zone)
< 0),
+ /* #### I have no idea what this conforms to,
+ but the compiler has stopped whining. */
+ sprintf (tzbuf, "XXX%s%d:%02d:%02d", (XFIXNUM (zone) < 0) ?
"-" : "+",
abszone / (60*60), (abszone/60) % 60, abszone % 60);
tzstring = tzbuf;
}
AA> http://git.savannah.gnu.org/cgit/emacs.git/tree/src/editfns.c#n1937
AA> suggests that they are not including a + character for positive offsets,
AA> which surprised me.
According to `man tzset', the + character is optional. I prefer to make
it explicit to avoid confusion.
AA> But zone offsets > 0 do not include the - sign, which is the same logic
AA> XEmacs uses before your patch.
No, it is not. GNU Emacs is correct, but it's implementation is a
rather hacky array access: &"-"[XINT (zone) < 0]
A negative zone offset yields &"-"[1], which skips the minus and yields
the empty string; a zero or positive zone offset yields the minus via
&"-"[0].
AA> Also, why are you removing the REST argument in passing without mention?
That was a misunderstanding on my part, sorry for that.
--
Marcus
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
12:24 p.m.
On 30.09.2014 19:28, Marcus Crestani wrote:
>>>>> "AA" == Adrian Aichner
<adrian.aichner(a)gmail.com> writes:
AA> How could XEmacs and GNU Emacs both be wrong until now?
XEmacs wasn't wrong for long, this caused the problem:
changeset: 5762:427a72c6ee17
user: Stephen J. Turnbull <stephen(a)xemacs.org>
date: Sun Sep 15 23:47:37 2013 +0900
summary: Eliminate several compiler (clang) warnings.
diff --git a/src/editfns.c b/src/editfns.c
--- a/src/editfns.c
+++ b/src/editfns.c
＠＠ -1190,7 +1190,9 ＠＠
else if (FIXNUMP (zone))
{
int abszone = abs (XFIXNUM (zone));
- sprintf (tzbuf, "XXX%s%d:%02d:%02d", "-" + (XFIXNUM (zone)
< 0),
+ /* #### I have no idea what this conforms to,
+ but the compiler has stopped whining. */
+ sprintf (tzbuf, "XXX%s%d:%02d:%02d", (XFIXNUM (zone) < 0) ?
"-" : "+",
abszone / (60*60), (abszone/60) % 60, abszone % 60);
tzstring = tzbuf;
}
AA> http://git.savannah.gnu.org/cgit/emacs.git/tree/src/editfns.c#n1937
AA> suggests that they are not including a + character for positive offsets,
AA> which surprised me.
According to `man tzset', the + character is optional. I prefer to make
it explicit to avoid confusion.
AA> But zone offsets > 0 do not include the - sign, which is the same logic
AA> XEmacs uses before your patch.
No, it is not. GNU Emacs is correct, but it's implementation is a
rather hacky array access: &"-"[XINT (zone) < 0]
I'm glad you said that.
A negative zone offset yields &"-"[1], which skips the minus and yields
the empty string; a zero or positive zone offset yields the minus via
&"-"[0].
Sorry for the false alarm. I even went through the steps you describe,
but I did it wrong.
I guess the ternary expression messed with a brain-half while I went
through the exercise with the other.
So the encoding has to apply the timezone offset in opposite direction
to arrive at the UTC value for the encoded time, right?
GNU Emacs, it appears, prefixes 0 with a minus sign as well.
AA> Also, why are you removing the REST argument in passing without mention?
That was a misunderstanding on my part, sorry for that.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
1:12 p.m.
>>>>"AA" == Adrian Aichner
<adrian.aichner(a)gmail.com> writes:
AA> So the encoding has to apply the
timezone offset in opposite direction
AA> to arrive at the UTC value for the encoded time, right?
Right.
AA> GNU Emacs, it appears, prefixes 0 with a minus sign as well.
Well, this doesn't hurt but is not as nice to the eye as "+0", IMHO. Do
you prefer that XEmacs should behave exactly like GNU Emacs and also add
a minus sign when the time offset is zero?
--
Marcus
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
2:30 p.m.
On 01.10.2014 14:12, Marcus Crestani wrote:
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
>>>>> "AA" == Adrian Aichner
<adrian.aichner(a)gmail.com> writes:
AA> So the encoding has to apply the timezone offset in opposite direction
AA> to arrive at the UTC value for the encoded time, right?
Right.
AA> GNU Emacs, it appears, prefixes 0 with a minus sign as well.
Well, this doesn't hurt but is not as nice to the eye as "+0", IMHO. Do
you prefer that XEmacs should behave exactly like GNU Emacs and also add
a minus sign when the time offset is zero?
No, I like our current formatting. Thanks for the confirmations.
3:12 a.m.
Marcus Crestani writes:
>>>>>"AA" == Adrian Aichner
<adrian.aichner(a)gmail.com> writes:
AA> So the encoding has to apply the timezone offset in opposite direction
AA> to arrive at the UTC value for the encoded time, right?
Right.
Yup, that's what I missed too, and worry about that is why I
complained about conformance in the comment.
AA> GNU Emacs, it appears, prefixes 0 with a minus sign as well.
Well, this doesn't hurt but is not as nice to the eye as "+0", IMHO. Do
you prefer that XEmacs should behave exactly like GNU Emacs and also add
a minus sign when the time offset is zero?
"+0" is prettier, but there may be code that looks for "-0"
explicitly
out there (eg, to test for GMT). If you're going to do it differently
from GNU, please document it.
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
7:12 a.m.
New subject: [S 21.5] Fix signs in time zone string.
SUPERSEDES PATCH 21.5
>>>>"SJT" == Stephen J Turnbull
<stephen(a)xemacs.org> writes:
SJT> "+0" is prettier, but there may
be code that looks for "-0" explicitly
SJT> out there (eg, to test for GMT). If you're going to do it differently
SJT> from GNU, please document it.
Sure, see below. Is the comment clear enough? Should it mention that
GNU Emacs does it differently?
diff --git a/src/editfns.c b/src/editfns.c
--- a/src/editfns.c
+++ b/src/editfns.c
＠＠ -1190,9 +1190,13 ＠＠
else if (FIXNUMP (zone))
{
int abszone = abs (XFIXNUM (zone));
- /* #### I have no idea what this conforms to,
- but the compiler has stopped whining. */
- sprintf (tzbuf, "XXX%s%d:%02d:%02d", (XFIXNUM (zone) < 0) ? "-"
: "+",
+ /* We specify the time zone in offset notation (see `man
+ tzset' for details). The offset indicates the value one
+ must add to local time to arrive at UTC. Thus, we sign
+ the offset with a `-' if the time zone is east of GMT; we
+ sign the offset with a `+' if the time zone is GMT (then
+ the offset is 0) or if the time zone is west of GMT. */
+ sprintf (tzbuf, "XXX%s%d:%02d:%02d", (XFIXNUM (zone) < 0) ? "+"
: "-",
abszone / (60*60), (abszone/60) % 60, abszone % 60);
tzstring = tzbuf;
}
--
Marcus
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
8:09 a.m.
New subject: [S 21.5] Fix signs in time zone string.
Marcus Crestani writes:
SUPERSEDES PATCH 21.5
>>>>>"SJT" == Stephen J Turnbull <stephen(a)xemacs.org>
writes:
SJT> "+0" is prettier, but there may be code that looks for "-0"
explicitly
SJT> out there (eg, to test for GMT). If you're going to do it differently
SJT> from GNU, please document it.
Sure, see below. Is the comment clear enough? Should it mention that
GNU Emacs does it differently?
I'm sorry, I meant in the docstring, but upon looking more closely
that makes no sense since in fact this string never is seen by a user
as far as I can tell. If that's true, the comment is great!
Steve
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
8:23 a.m.
New subject: [S 21.5] Fix signs in time zone string.
>>>>"SJT" == Stephen J Turnbull
<stephen(a)xemacs.org> writes:
SJT> I'm sorry, I meant in the docstring,
but upon looking more closely
SJT> that makes no sense since in fact this string never is seen by a user
SJT> as far as I can tell.
Yes, the string is never seen by a user.
--
Marcus
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
8:55 a.m.
New subject: [A 21.5] Fix signs in time zone string.
APPROVE
Go for it!
Marcus Crestani writes:
>>>>>"SJT" == Stephen J Turnbull
<stephen(a)xemacs.org> writes:
SJT> I'm sorry, I meant in the docstring, but upon looking more closely
SJT> that makes no sense since in fact this string never is seen by a user
SJT> as far as I can tell.
Yes, the string is never seen by a user.
--
Marcus
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
9:20 a.m.
New subject: [COMMIT 21.5] Fix signs in time zone string.
COMMIT 21.5
>>>>"SJT" == Stephen J Turnbull
<stephen(a)xemacs.org> writes:
SJT> APPROVE
SJT> Go for it!
Thanks, Stephen!
--
Marcus
_______________________________________________
XEmacs-Patches mailing list
XEmacs-Patches(a)xemacs.org
http://lists.xemacs.org/mailman/listinfo/xemacs-patches
4002
days inactive
4004
days old
10 comments
3 participants
participants (3)
-
Adrian Aichner -
Marcus Crestani -
Stephen J. Turnbull