* (0 <= i && i < N) is not "backwards" @ 2013-03-24 23:14 Paul Eggert 2013-03-24 23:53 ` Pascal J. Bourguignon ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Paul Eggert @ 2013-03-24 23:14 UTC (permalink / raw) To: Andreas Schwab; +Cc: Emacs Development Emacs trunk bzr 112126, installed with the commit log "Reorder conditions that are written backwards", changed a lot of Emacs code, typically to replace expressions like "0 < i" with expressions like "i > 0". This sort of stylistic change shouldn't be introduced without discussion. I often prefer "<", as it causes textual order to reflect numeric order. This is not just a personal preference; it's a common style used in other GNU projects. Removing this style en masse is not called for, particularly in places where the code is checking for values in range. In one or two places the change may have introduced a bug, as "! (0 < X)" is not equivalent to "X <= 0" when X is floating point, because of NaNs. I suggest reverting the change and discussing it before applying. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-24 23:14 (0 <= i && i < N) is not "backwards" Paul Eggert @ 2013-03-24 23:53 ` Pascal J. Bourguignon 2013-03-25 14:23 ` Stefan Monnier 2013-03-25 8:20 ` Eli Zaretskii 2013-03-30 21:45 ` Jim Meyering 2 siblings, 1 reply; 13+ messages in thread From: Pascal J. Bourguignon @ 2013-03-24 23:53 UTC (permalink / raw) To: emacs-devel Paul Eggert <eggert@cs.ucla.edu> writes: > Emacs trunk bzr 112126, installed with the commit log > "Reorder conditions that are written backwards", > changed a lot of Emacs code, typically to replace expressions > like "0 < i" with expressions like "i > 0". > > This sort of stylistic change shouldn't be introduced without > discussion. I often prefer "<", as it causes textual > order to reflect numeric order. This is not just a personal > preference; it's a common style used in other GNU projects. > Removing this style en masse is not called for, particularly > in places where the code is checking for values in range. Indeed, writing all comparisons only with < or <= is one of the only two things I learned about programming from university. (The other one being to write IFs so that the error case is processed first (assumedly being smaller)). Also, for lispers: in lisp, we write: (plusp i), not (i plusp). Hence: (0< i), not (i >0). (null r) Hence: (0== r) > In one or two places the change may have introduced a bug, > as "! (0 < X)" is not equivalent to "X <= 0" when > X is floating point, because of NaNs. > > I suggest reverting the change and discussing it before > applying. -- __Pascal Bourguignon__ http://www.informatimago.com/ A bad day in () is better than a good day in {}. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-24 23:53 ` Pascal J. Bourguignon @ 2013-03-25 14:23 ` Stefan Monnier 2013-03-25 14:59 ` Andreas Schwab 0 siblings, 1 reply; 13+ messages in thread From: Stefan Monnier @ 2013-03-25 14:23 UTC (permalink / raw) To: Pascal J. Bourguignon; +Cc: emacs-devel > Hence: (0== r) For equality comparisons, there's another reason to do "2 == x" rather than "x == 2", which is that if you happen to miswrite it as "2 = x", the compiler will detect the error. Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 14:23 ` Stefan Monnier @ 2013-03-25 14:59 ` Andreas Schwab 0 siblings, 0 replies; 13+ messages in thread From: Andreas Schwab @ 2013-03-25 14:59 UTC (permalink / raw) To: Stefan Monnier; +Cc: Pascal J. Bourguignon, emacs-devel Stefan Monnier <monnier@iro.umontreal.ca> writes: > For equality comparisons, there's another reason to do "2 == x" rather > than "x == 2", which is that if you happen to miswrite it as "2 = x", the > compiler will detect the error. Fortunately, the emacs source is free from this ugliness. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-24 23:14 (0 <= i && i < N) is not "backwards" Paul Eggert 2013-03-24 23:53 ` Pascal J. Bourguignon @ 2013-03-25 8:20 ` Eli Zaretskii 2013-03-25 14:35 ` Paul Eggert 2013-03-30 21:45 ` Jim Meyering 2 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2013-03-25 8:20 UTC (permalink / raw) To: Paul Eggert; +Cc: schwab, Emacs-devel > Date: Sun, 24 Mar 2013 16:14:49 -0700 > From: Paul Eggert <eggert@cs.ucla.edu> > Cc: Emacs Development <Emacs-devel@gnu.org> > > Emacs trunk bzr 112126, installed with the commit log > "Reorder conditions that are written backwards", > changed a lot of Emacs code, typically to replace expressions > like "0 < i" with expressions like "i > 0". > > This sort of stylistic change shouldn't be introduced without > discussion. Out of fairness, you introduced this style into Emacs sources in the first place without any discussions that I could remember or find in the archives. If we want to discuss stylistic changes before committing them, let's do that no matter who is the committer. > In one or two places the change may have introduced a bug, > as "! (0 < X)" is not equivalent to "X <= 0" when > X is floating point, because of NaNs. If we want our code to be robust in the face of NaNs, we should probably use 'isnan' explicitly, because (as you know very well) every comparison with a NaN yields zero, a.k.a. false. OTOH, whether a NaN should be considered greater or less than zero in the Emacs context is debatable, IMO. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 8:20 ` Eli Zaretskii @ 2013-03-25 14:35 ` Paul Eggert 2013-03-25 14:53 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Paul Eggert @ 2013-03-25 14:35 UTC (permalink / raw) To: Eli Zaretskii; +Cc: schwab, Emacs-devel On 03/25/2013 01:20 AM, Eli Zaretskii wrote: > Out of fairness, you introduced this style into Emacs sources I've certainly used it in the changes I made, and as a result it's become more popular, but I did not introduce it. That style has been used in Emacs, as a minority style, for many years. > If we want our code to be robust in the face of NaNs, we should > probably use 'isnan' explicitly That will slow the code down and make it harder to read. Perhaps a comment could be introduced; but must we really add a comment "watch out for NaNs!" every time we have a floating point comparison? > whether a NaN > should be considered greater or less than zero in the Emacs context is > debatable The documentation for one of the affected variables says "If the value is not a number, ...", so the behavior formerly matched the documentation and now no longer does so. This was not the only bug introduced by the change, I'm afraid. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 14:35 ` Paul Eggert @ 2013-03-25 14:53 ` Eli Zaretskii 2013-03-25 14:58 ` Noah Lavine 2013-03-25 16:49 ` Paul Eggert 0 siblings, 2 replies; 13+ messages in thread From: Eli Zaretskii @ 2013-03-25 14:53 UTC (permalink / raw) To: Paul Eggert; +Cc: schwab, Emacs-devel > Date: Mon, 25 Mar 2013 07:35:51 -0700 > From: Paul Eggert <eggert@cs.ucla.edu> > CC: schwab@linux-m68k.org, Emacs-devel@gnu.org > > On 03/25/2013 01:20 AM, Eli Zaretskii wrote: > > > Out of fairness, you introduced this style into Emacs sources > > I've certainly used it in the changes I made, and as a result it's > become more popular, but I did not introduce it. That style has > been used in Emacs, as a minority style, for many years. AFAIR, it was once a negligible minority at best, so much so that I don't remember it being used. That was in the days that the only style enforced in the Emacs sources was what the GNU Coding Standards say, btw. That's what allowed "minority styles" in the first place. > > If we want our code to be robust in the face of NaNs, we should > > probably use 'isnan' explicitly > > That will slow the code down and make it harder to read. I disagree; there's nothing unclear in a call to isnan, and the slow-down is negligible, if it exists at all, and won't be noticed in the context of Emacs, which doesn't pretend to be a fast number-cruncher anyway. > Perhaps a > comment could be introduced; but must we really add a comment > "watch out for NaNs!" every time we have a floating point comparison? Such comments would be even worse. But the problem with relying on 0 < foo is that most people won't even consider the subtle difference between that and !(foo >= 0) when NaNs are involved. Calling isnan makes that explicit. Anyway, this issue is tangential to the style issue. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 14:53 ` Eli Zaretskii @ 2013-03-25 14:58 ` Noah Lavine 2013-03-25 15:09 ` Eli Zaretskii 2013-03-25 16:49 ` Paul Eggert 1 sibling, 1 reply; 13+ messages in thread From: Noah Lavine @ 2013-03-25 14:58 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Paul Eggert, Andreas Schwab, Emacs development discussions [-- Attachment #1: Type: text/plain, Size: 2022 bytes --] Hello, To continue a tangent, if adding an explicit isnan() makes the code run any slower, I think you should file a bug with GCC to get that fixed. In general, I really dislike the idea of adjusting coding style to work around a compiler's limitations. If the compiler is broken, I think it's best to fix it and move on. Thanks, Noah Lavine On Mon, Mar 25, 2013 at 10:53 AM, Eli Zaretskii <eliz@gnu.org> wrote: > > Date: Mon, 25 Mar 2013 07:35:51 -0700 > > From: Paul Eggert <eggert@cs.ucla.edu> > > CC: schwab@linux-m68k.org, Emacs-devel@gnu.org > > > > On 03/25/2013 01:20 AM, Eli Zaretskii wrote: > > > > > Out of fairness, you introduced this style into Emacs sources > > > > I've certainly used it in the changes I made, and as a result it's > > become more popular, but I did not introduce it. That style has > > been used in Emacs, as a minority style, for many years. > > AFAIR, it was once a negligible minority at best, so much so that I > don't remember it being used. That was in the days that the only > style enforced in the Emacs sources was what the GNU Coding Standards > say, btw. That's what allowed "minority styles" in the first place. > > > > If we want our code to be robust in the face of NaNs, we should > > > probably use 'isnan' explicitly > > > > That will slow the code down and make it harder to read. > > I disagree; there's nothing unclear in a call to isnan, and the > slow-down is negligible, if it exists at all, and won't be noticed in > the context of Emacs, which doesn't pretend to be a fast > number-cruncher anyway. > > > Perhaps a > > comment could be introduced; but must we really add a comment > > "watch out for NaNs!" every time we have a floating point comparison? > > Such comments would be even worse. > > But the problem with relying on 0 < foo is that most people won't even > consider the subtle difference between that and !(foo >= 0) when NaNs > are involved. Calling isnan makes that explicit. > > Anyway, this issue is tangential to the style issue. > > [-- Attachment #2: Type: text/html, Size: 2834 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 14:58 ` Noah Lavine @ 2013-03-25 15:09 ` Eli Zaretskii 2013-03-25 15:28 ` Noah Lavine 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2013-03-25 15:09 UTC (permalink / raw) To: Noah Lavine; +Cc: eggert, schwab, Emacs-devel > From: Noah Lavine <noah.b.lavine@gmail.com> > Date: Mon, 25 Mar 2013 10:58:28 -0400 > Cc: Paul Eggert <eggert@cs.ucla.edu>, Andreas Schwab <schwab@linux-m68k.org>, > Emacs development discussions <Emacs-devel@gnu.org> > > To continue a tangent, if adding an explicit isnan() makes the code run any > slower, I think you should file a bug with GCC to get that fixed. I don't think GCC is a problem: isnan is a builtin function there, so even a function call overhead is spared. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 15:09 ` Eli Zaretskii @ 2013-03-25 15:28 ` Noah Lavine 0 siblings, 0 replies; 13+ messages in thread From: Noah Lavine @ 2013-03-25 15:28 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Paul Eggert, Andreas Schwab, Emacs development discussions [-- Attachment #1: Type: text/plain, Size: 802 bytes --] On Mon, Mar 25, 2013 at 11:09 AM, Eli Zaretskii <eliz@gnu.org> wrote: > > From: Noah Lavine <noah.b.lavine@gmail.com> > > Date: Mon, 25 Mar 2013 10:58:28 -0400 > > Cc: Paul Eggert <eggert@cs.ucla.edu>, Andreas Schwab < > schwab@linux-m68k.org>, > > Emacs development discussions <Emacs-devel@gnu.org> > > > > To continue a tangent, if adding an explicit isnan() makes the code run > any > > slower, I think you should file a bug with GCC to get that fixed. > > I don't think GCC is a problem: isnan is a builtin function there, so > even a function call overhead is spared. > > Oh, that's good. I didn't know how it was implemented. I mean that GCC should be able to convert ((x > 0) || isnan(x)) to !(x <= 0) if the second expression is actually faster. It seems like a fairly simple rewrite. [-- Attachment #2: Type: text/html, Size: 1398 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 14:53 ` Eli Zaretskii 2013-03-25 14:58 ` Noah Lavine @ 2013-03-25 16:49 ` Paul Eggert 2013-03-25 16:59 ` Eli Zaretskii 1 sibling, 1 reply; 13+ messages in thread From: Paul Eggert @ 2013-03-25 16:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: schwab, Emacs-devel On 03/25/13 07:53, Eli Zaretskii wrote: > the problem with relying on 0 < foo is that most people won't even > consider the subtle difference between that and !(foo >= 0) There must be dozens of places in the Emacs source code that use floating-point comparisons and rely on NaNs behaving the way that they do. It'd be unnecessary clutter to add comments and/or isnan() calls to them all. Sure, we could modify our compilers (and this would include the Emacs Lisp compiler) to recognize and optimize-away the isnan calls, but that would be even more make-work. Programmers who deal with floating-point should be cognizant of NaNs; that's just life in the floating-point big city these days. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-25 16:49 ` Paul Eggert @ 2013-03-25 16:59 ` Eli Zaretskii 0 siblings, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2013-03-25 16:59 UTC (permalink / raw) To: Paul Eggert; +Cc: schwab, Emacs-devel > Date: Mon, 25 Mar 2013 09:49:28 -0700 > From: Paul Eggert <eggert@cs.ucla.edu> > CC: schwab@linux-m68k.org, Emacs-devel@gnu.org > > On 03/25/13 07:53, Eli Zaretskii wrote: > > the problem with relying on 0 < foo is that most people won't even > > consider the subtle difference between that and !(foo >= 0) > > There must be dozens of places in the Emacs source code that > use floating-point comparisons and rely on NaNs behaving the > way that they do. I'd be surprised if there were literally "dozens" of such places. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (0 <= i && i < N) is not "backwards" 2013-03-24 23:14 (0 <= i && i < N) is not "backwards" Paul Eggert 2013-03-24 23:53 ` Pascal J. Bourguignon 2013-03-25 8:20 ` Eli Zaretskii @ 2013-03-30 21:45 ` Jim Meyering 2 siblings, 0 replies; 13+ messages in thread From: Jim Meyering @ 2013-03-30 21:45 UTC (permalink / raw) To: Paul Eggert; +Cc: Andreas Schwab, Emacs Development Paul Eggert wrote: > Emacs trunk bzr 112126, installed with the commit log > "Reorder conditions that are written backwards", > changed a lot of Emacs code, typically to replace expressions > like "0 < i" with expressions like "i > 0". > > This sort of stylistic change shouldn't be introduced without > discussion. I often prefer "<", as it causes textual I was surprised to see that commit, and came to this list to find the discussion that happened prior to the global style conversion. Considering that there was no discussion, a snap revert would be fair. Full disclosure: I too prefer the "use < and <= everywhere" style, and mention it in the GNU coreutils' HACKING guidelines: http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n412 Jim ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-03-30 21:45 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-24 23:14 (0 <= i && i < N) is not "backwards" Paul Eggert 2013-03-24 23:53 ` Pascal J. Bourguignon 2013-03-25 14:23 ` Stefan Monnier 2013-03-25 14:59 ` Andreas Schwab 2013-03-25 8:20 ` Eli Zaretskii 2013-03-25 14:35 ` Paul Eggert 2013-03-25 14:53 ` Eli Zaretskii 2013-03-25 14:58 ` Noah Lavine 2013-03-25 15:09 ` Eli Zaretskii 2013-03-25 15:28 ` Noah Lavine 2013-03-25 16:49 ` Paul Eggert 2013-03-25 16:59 ` Eli Zaretskii 2013-03-30 21:45 ` Jim Meyering
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.