(0 <= i && i < N) is not "backwards"

(0 <= i && i < N) is not "backwards"

[Paul Eggert]

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.

Re: (0 <= i && i < N) is not "backwards"

[Pascal J. Bourguignon]

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 {}.

Re: (0 <= i && i < N) is not "backwards"

[Stefan Monnier]

> 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

Re: (0 <= i && i < N) is not "backwards"

[Andreas Schwab]

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."

Re: (0 <= i && i < N) is not "backwards"

[Eli Zaretskii]

> 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.

Re: (0 <= i && i < N) is not "backwards"

[Paul Eggert]

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.

Re: (0 <= i && i < N) is not "backwards"

[Eli Zaretskii]

> 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.

Re: (0 <= i && i < N) is not "backwards"

[Noah Lavine]

[-- 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 --]

Re: (0 <= i && i < N) is not "backwards"

[Eli Zaretskii]

> 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.

Re: (0 <= i && i < N) is not "backwards"

[Noah Lavine]

[-- 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 --]

Re: (0 <= i && i < N) is not "backwards"

[Paul Eggert]

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.

Re: (0 <= i && i < N) is not "backwards"

[Eli Zaretskii]

> 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.

Re: (0 <= i && i < N) is not "backwards"

[Jim Meyering]

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