Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Stefan Monnier]

>     min and max now return one of their arguments
    
Since this is an incompatible change, I'm wondering what is the
motivation for this change.  I can see advantages to both behaviors, but
I don't see an overriding advantage that would justify getting over the
inertia and breaking backward compatibility.


        Stefan

Re: master 37940b3: min and max now return one of their arguments

[Glenn Morris]

Paul Eggert wrote:

> branch: master
> commit 37940b347052418f0589bd52b06e56fffb594ea2
>
>     min and max now return one of their arguments


It seems that this change causes a bunch of tests to fail with

(wrong-type-argument numberp #<marker in no buffer>)

errors.

Ref: http://hydra.nixos.org/build/49801480

(the previous coverage build also failed, due to bug#24503, I think)

I reverted the commit locally and the tests pass again.

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> min and max now return one of their arguments

SM> Since this is an incompatible change, I'm wondering what is the motivation
SM> for this change. I can see advantages to both behaviors, but I don't see
SM> an overriding advantage that would justify getting over the inertia and
SM> breaking backward compatibility.

Yes, same here, I would like to know this as well.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]

Re: master 37940b3: min and max now return one of their arguments

[Tino Calancha]

On Tue, 7 Mar 2017, Glenn Morris wrote:

> Paul Eggert wrote:
>
>> branch: master
>> commit 37940b347052418f0589bd52b06e56fffb594ea2
>>
>>     min and max now return one of their arguments
>
>
> It seems that this change causes a bunch of tests to fail with
>
> (wrong-type-argument numberp #<marker in no buffer>)
One example of regression:
emacs -Q lisp/subr.el
C-x h TAB
;; Signal error:
progress-reporter-do-update: Wrong type argument: numberp, #<marker at 211140 in subr.el>

Re: master 37940b3: min and max now return one of their arguments

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

Glenn Morris wrote:
> It seems that this change causes a bunch of tests to fail with
>
> (wrong-type-argument numberp #<marker in no buffer>)

Sorry about that. Apparently 'min' and 'max' converted any marker result to an 
integer. I changed them back to do that with the attached patch. This behavior 
is undocumented; I don't know whether it was intended, although it looks like 
some code relies on it now.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-min-and-max-should-not-return-markers.patch --]
[-- Type: text/x-diff; name="0001-min-and-max-should-not-return-markers.patch", Size: 2110 bytes --]

From 79825fd06cfd7140c8d6e25db1c0fdc5a42f6098 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 7 Mar 2017 21:23:08 -0800
Subject: [PATCH] min and max should not return markers

Problem reported by Glenn Morris in:
http://lists.gnu.org/archive/html/emacs-devel/2017-03/msg00147.html
* src/data.c (minmax_driver): Convert any marker result to an
integer, since some callers assume this.
* test/src/data-tests.el (data-tests-max, data-tests-min):
Test for this.
---
 src/data.c             | 4 ++--
 test/src/data-tests.el | 8 ++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/data.c b/src/data.c
index 29547d8..c480ed2 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2972,9 +2972,9 @@ minmax_driver (ptrdiff_t nargs, Lisp_Object *args,
       if (argnum == 0 || !NILP (arithcompare (val, accum, comparison)))
 	accum = val;
       else if (FLOATP (accum) && isnan (XFLOAT_DATA (accum)))
-	break;
+	return accum;
     }
-  return accum;
+  return MARKERP (accum) ? make_number (marker_position (accum)) : accum;
 }
 
 DEFUN ("max", Fmax, Smax, 1, MANY, 0,
diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index 70ffdab..67d00a7 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -88,7 +88,9 @@
   (should (= (1+ most-negative-fixnum)
              (max (float most-negative-fixnum) (1+ most-negative-fixnum))))
   (should (= 8 (apply #'max '(3 8 3))))
-  (should-error (max 9 8 'foo)))
+  (should-error (max 9 8 'foo))
+  (should-error (max (make-marker)))
+  (should (eql 1 (max (point-min-marker) 1))))
 
 (ert-deftest data-tests-min ()
   (should-error (min))
@@ -98,7 +100,9 @@
   (should (= most-positive-fixnum
              (min (+ 1.0 most-positive-fixnum) most-positive-fixnum)))
   (should (= 3 (apply #'min '(3 8 3))))
-  (should-error (min 9 8 'foo)))
+  (should-error (min 9 8 'foo))
+  (should-error (min (make-marker)))
+  (should (eql 1 (min (point-min-marker) 1))))
 
 ;; Bool vector tests.  Compactly represent bool vectors as hex
 ;; strings.
-- 
2.9.3

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Paul Eggert]

Stefan Monnier wrote:
> I'm wondering what is the motivation for this change.

It avoids nonsensical behavior where 'min' and 'max' were inconsistent with '<' 
and '>'. Before the patch, (max A B) could return A even when (< A B) returned 
t. For example:

   (let* ((a 1e16)
          (b (1+ (floor a))))
     (list a b (max a b) (< a b) (if (< a b) b a)))

returned (1e+16 10000000000000001 1e+16 t 10000000000000001) on a 64-bit 
GNU/Linux host just before the patch was applied. In this example, '<' is 
behaving correctly since A is less than B, and 'max' is behaving incorrectly 
since it is returning the lesser of A and B. (Come to think of it, this example 
is clearer than what's in etc/NEWS, so I'll update etc/NEWS.)

In the old days Emacs could assume that every Emacs integer had an exact 
floating-point representation, so it was OK for functions like '<' and 'max' to 
convert integer arguments to floating-point before doing their comparisons since 
these conversions could not lose information. This is no longer true, and the 
low-level code now needs to be more careful when comparing fixnums to floats.

Re: master 37940b3: min and max now return one of their arguments

[Andreas Schwab]

On Mär 07 2017, Paul Eggert <eggert@cs.ucla.edu> wrote:

> Sorry about that. Apparently 'min' and 'max' converted any marker result
> to an integer.

Every arithmetic function does that.

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: master 37940b3: min and max now return one of their arguments

[Andreas Schwab]

On Mär 07 2017, Paul Eggert <eggert@cs.ucla.edu> wrote:

> This behavior is undocumented;

??? You removed the documentation!

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: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 7 Mar 2017 22:08:39 -0800
> 
> Stefan Monnier wrote:
> 
>     I'm wondering what is the motivation for this change.
> 
> It avoids nonsensical behavior where 'min' and 'max' were inconsistent with '<' and '>'. Before the patch, (max A B) could return A even when (< A B) returned t. For example:
> 
>   (let* ((a 1e16)
>          (b (1+ (floor a))))
>     (list a b (max a b) (< a b) (if (< a b) b a)))
> 
> returned (1e+16 10000000000000001 1e+16 t 10000000000000001) on a 64-bit GNU/Linux host just before the patch was applied.

That's because you recently changed how '<' works; before those
changes, '<' would report that a is neither less nor greater than b,
and thus was consistent with 'max'.  With a binary from last month, I
get this:

   (1e+16 10000000000000001 1e+16 nil 1e+16)

which shows that '<' and 'max' were consistent then.

> In this example, '<' is behaving correctly since A is less than B, and 'max' is behaving incorrectly since it is returning the lesser of A and B. (Come to think of it, this example is clearer than what's in etc/NEWS, so I'll update etc/NEWS.)

See above: this example will not necessarily make sense to someone who
upgrades from Emacs 25.

> In the old days Emacs could assume that every Emacs integer had an exact floating-point representation, so it was OK for functions like '<' and 'max' to convert integer arguments to floating-point before doing their comparisons since these conversions could not lose information. This is no longer true, and the low-level code now needs to be more careful when comparing fixnums to floats.

I'm not sure I like these changes in behavior.  Don't programmers
assume in general that comparisons between values some of which are
floats are always done after converting _all_ values to the
floating-point representation?  I, FWIW, do expect that, but maybe
Lisp programmers whose background is not C or Fortran don't.  In those
other languages, the above values of a and b cannot be meaningfully
compared in double floating-point representation, because the relative
difference between them is less than the machine epsilon.  IOW, these
two values are equal when represented as doubles.  Under this POV,
programs that want to take advantage of a wider integer data type are
supposed to manually convert them to integers, e.g. with 'floor',
before comparing or taking their 'max'.

Your changes break those expectations.  For example, the following C
program

  #include <stdio.h>

  int
  main (void)
  {
    double a = 1.e16;
    long long b = 10000000000000001LL;

    if (a < b)
      printf ("a < b\n");
    else if (a > b)
      printf ("a > b\n");
    else
      printf ("a = b\n");

    return 0;
  }

prints "a = b".  After your changes, the equivalent Lisp program will
behave differently.  Is this a good idea?

P.S. I find the commentary in 'arithcompare' not detailed enough to
explain to a casual reader some tricky code such as this:

	i1 = f2 = i2 = XINT (num2);
      fneq = f1 != f2;

The general idea is explained, but the devil is in the details, and
those are not commented at all.  Please consider adding more
explanations, so that the implementation becomes easier to grasp.  I
don't think it's a good idea to have our code present puzzles to the
reader, especially a reader who is not privy to the subtleties of FP
calculations.  TIA.

Re: master 37940b3: min and max now return one of their arguments

[Paul Eggert]

On 03/08/2017 12:29 AM, Andreas Schwab wrote:
> ??? You removed the documentation!

Ah, sorry, right you are; I'd forgotten that. Thanks for restoring it.

However, I don't understand the point of the code part of your change to 
minmax_driver. Since arithcompare already checks that its arguments are 
numbers or markers, it's redundant for minmax driver to do so as well, 
so CHECK_NUMBER_OR_FLOAT_COERCE_MARKER call that you added to 
arithcompare will slow Emacs down slightly. Is there some reason why the 
slower code is necessary?

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On 03/08/2017 07:54 AM, Eli Zaretskii wrote:
> Don't programmers
> assume in general that comparisons between values some of which are
> floats are always done after converting_all_  values to the
> floating-point representation?

No, and that hasn't been true for Emacs Lisp for quite some time. In 
Emacs 25, (<= 10000000000000001 10000000000000000 1e16) returns the 
mathematically-correct answer nil on a 64-bit GNU/Linux host even though 
it would return t if all values were first converted to float.

More generally, although people expect statically typed languages to 
convert alternatives to float (e.g., C programmers expect (1.5 > 2 ? 1.5 
: 2) to return 2.0 not 2), dynamically typed languages are different: 
Lisp programmers expect (if (> 1.5 2) 1.5 2) to return 2 not 2.0.

Admittedly the situation with min and max is not quite as simple as with 
'if' (which clearly should not convert). Still, it's verrrry strange for 
(< A (min A B)) to return t, and having 'min' return the numerically 
correct answer avoids nonsensical behavior like that.


> I find the commentary in 'arithcompare' not detailed enough
I installed the attached, which I hope helps.


[-- Attachment #2: 0001-src-data.c-arithcompare-Add-comments.patch --]
[-- Type: application/x-patch, Size: 2203 bytes --]

Re: master 37940b3: min and max now return one of their arguments

[Andreas Schwab]

On Mär 08 2017, Paul Eggert <eggert@cs.ucla.edu> wrote:

> Is there some reason why the slower code is necessary?

Clarity and consistency.

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: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 8 Mar 2017 14:04:54 -0800
> 
> On 03/08/2017 07:54 AM, Eli Zaretskii wrote:
> > Don't programmers
> > assume in general that comparisons between values some of which are
> > floats are always done after converting_all_  values to the
> > floating-point representation?
> 
> No, and that hasn't been true for Emacs Lisp for quite some time. In 
> Emacs 25, (<= 10000000000000001 10000000000000000 1e16) returns the 
> mathematically-correct answer nil on a 64-bit GNU/Linux host even though 
> it would return t if all values were first converted to float.

Not sure what that demonstrates, but I get

  (<= 10000000000000001 1e16) => t

in Emacs 25.1 on a 64-bit GNU/Linux host.

> More generally, although people expect statically typed languages to 
> convert alternatives to float (e.g., C programmers expect (1.5 > 2 ? 1.5 
> : 2) to return 2.0 not 2), dynamically typed languages are different: 
> Lisp programmers expect (if (> 1.5 2) 1.5 2) to return 2 not 2.0.

I'm not sure this is so, and anyway getting 2.0 should not matter in
Emacs Lisp.

> > I find the commentary in 'arithcompare' not detailed enough
> I installed the attached, which I hope helps.

Thanks.

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Clément Pit-Claudel]

On 2017-03-09 11:10, Eli Zaretskii wrote:
> 
>> More generally, although people expect statically typed languages to 
>> convert alternatives to float (e.g., C programmers expect (1.5 > 2 ? 1.5 
>> : 2) to return 2.0 not 2), dynamically typed languages are different: 
>> Lisp programmers expect (if (> 1.5 2) 1.5 2) to return 2 not 2.0.
>
> I'm not sure this is so, and anyway getting 2.0 should not matter in
> Emacs Lisp.

I think it does matter sometimes, as some bits of Emacs explicitly distinguish between floats and ints (for example I might write `(face (:height ,(if use-absolute-size 120 1.2))))

Clément.
 

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Eli Zaretskii]

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Thu, 9 Mar 2017 11:25:22 -0500
> 
> I think it does matter sometimes, as some bits of Emacs explicitly distinguish between floats and ints (for example I might write `(face (:height ,(if use-absolute-size 120 1.2))))

If that's the use case, you will have to use the likes of 'floor'
anyway, because you cannot control well enough what happens in
non-trivial calculations involving floats.

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Clément Pit-Claudel]

On 2017-03-09 11:54, Eli Zaretskii wrote:
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Thu, 9 Mar 2017 11:25:22 -0500
>>
>> I think it does matter sometimes, as some bits of Emacs explicitly distinguish between floats and ints (for example I might write `(face (:height ,(if use-absolute-size 120 1.2))))
> 
> If that's the use case, you will have to use the likes of 'floor'
> anyway, because you cannot control well enough what happens in
> non-trivial calculations involving floats.

Sorry, I don't have an opinion on the larger context discussed here.  I was just pointing that it's common in ELisp to expect 'if' not to coerce its arguments.

Cheers,
Clément.

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Paul Eggert]

On 03/09/2017 08:10 AM, Eli Zaretskii wrote:
>>> Don't programmers
>>> assume in general that comparisons between values some of which are
>>> floats are always done after converting_all_  values to the
>>> floating-point representation?
>> No, and that hasn't been true for Emacs Lisp for quite some time. In
>> Emacs 25, (<= 10000000000000001 10000000000000000 1e16) returns the
>> mathematically-correct answer nil on a 64-bit GNU/Linux host even though
>> it would return t if all values were first converted to float.
> Not sure what that demonstrates

It demonstrates that historically Emacs has been inconsistent in this 
area. Sometimes it behaves as if it converts all values to 
floating-point, sometimes not. The example I gave is where Emacs 25 
returns the mathematically-correct answer even though converting all 
values to floating-point would return the mathematically-incorrect 
answer. There are other examples where Emacs 25 does the opposite.

> I get
>
>    (<= 10000000000000001 1e16) => t
>
> in Emacs 25.1 on a 64-bit GNU/Linux host.
>
Yes, and that's an example of the opposite. Emacs 25 is inconsistent, 
and this inconsistency leads to weirdnesses, e.g., it's possible for (<= 
A C) to be nil even though (<= A B C) is t, because the implementation 
of <= does not compare consistently. Programmers generally do not expect 
these weirdnesses, and the patches in master remove them by having Emacs 
use consistent comparisons.

Re: [Emacs-diffs] master 37940b3: min and max now return one of their arguments

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 9 Mar 2017 09:45:47 -0800
> 
> > I get
> >
> >    (<= 10000000000000001 1e16) => t
> >
> > in Emacs 25.1 on a 64-bit GNU/Linux host.
> >
> Yes, and that's an example of the opposite. Emacs 25 is inconsistent, 

It doesn't look like inconsistency to me, because I expect these two
numbers to be indistinguishable.  It's basically the same as getting
unstable results from qsort -- this should be expected.

> and this inconsistency leads to weirdnesses, e.g., it's possible for (<= 
> A C) to be nil even though (<= A B C) is t, because the implementation 
> of <= does not compare consistently. Programmers generally do not expect 
> these weirdnesses, and the patches in master remove them by having Emacs 
> use consistent comparisons.

Programmers who know something about FP calculations do expect that,
and won't be surprised.

Re: master 37940b3: min and max now return one of their arguments

[Andreas Politz]

Is this also related ? 

#+BEGIN_SRC emacs-lisp
  (setq debug-on-error t)
  (with-temp-buffer
    (insert "foo")
    (emacs-lisp-mode)
    (indent-region (point-min) (point-max)))
#+END_SRC

Debugger entered--Lisp error: (wrong-type-argument numberp #<marker at 4 in  *temp*>)
  ceiling(#<marker at 4 in  *temp*>)
  progress-reporter-do-update((#<marker at 4 in  *temp*> . [1489243182.6902182 1 #<marker at 4 in  *temp*> "Indenting region..." 1 0.2]) 4)
  indent-region-line-by-line(1 4)
  indent-region(1 4)

-ap

Re: master 37940b3: min and max now return one of their arguments

[Tino Calancha]

On Sat, 11 Mar 2017, Andreas Politz wrote:

>
> Is this also related ?
Yes, it is.  It was fixed in commit:
1590127335cef06af8d77884f9c16a8629d65943
> #+BEGIN_SRC emacs-lisp
>  (setq debug-on-error t)
>  (with-temp-buffer
>    (insert "foo")
>    (emacs-lisp-mode)
>    (indent-region (point-min) (point-max)))
> #+END_SRC
>
> Debugger entered--Lisp error: (wrong-type-argument numberp #<marker at 4 in  *temp*>)
>  ceiling(#<marker at 4 in  *temp*>)
>  progress-reporter-do-update((#<marker at 4 in  *temp*> . [1489243182.6902182 1 #<marker at 4 in  *temp*> "Indenting region..." 1 0.2]) 4)
>  indent-region-line-by-line(1 4)
>  indent-region(1 4)
>
> -ap
>
>

Re: master 37940b3: min and max now return one of their arguments

[Andreas Politz]

Tino Calancha <tino.calancha@gmail.com> writes:

> On Sat, 11 Mar 2017, Andreas Politz wrote:
>
>>
>> Is this also related ?
> Yes, it is.  It was fixed in commit:
> 1590127335cef06af8d77884f9c16a8629d65943

Ok, thanks.

-ap