GNU bug report logs - #34781
27.0.50; integer in pcase sometimes compared by eq

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 34781 in the body.
You can then email your comments to 34781 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 07 Mar 2019 16:13:27 +0100

(defun f (x)
  (pcase x
    ((or 'a #x10000000000000000) t)))

(f #x10000000000000000) => nil

Just replacing integerp with fixnump fixes this, but I'm not sure if
more of the same lurks somewhere.

--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -792,7 +792,7 @@ pcase--u1
                    (let ((upat (cddr alt)))
                      (eq (car-safe upat) 'quote)))
               (let ((val (cadr (cddr alt))))
-                (unless (or (integerp val) (symbolp val))
+                (unless (or (fixnump val) (symbolp val))
                   (setq memq-ok nil))
                 (push (cadr (cddr alt)) simples))
             (push alt others))))

Message #8 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: Acknowledgement (27.0.50; integer in pcase sometimes
 compared by eq)
Date: Tue, 12 Mar 2019 13:24:41 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

Complete patch with test case.

[0001-Don-t-match-bignums-with-memq-in-pcase.patch (text/x-patch, attachment)]

Message #13 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: 34781 <at> debbugs.gnu.org
Subject: 27.0.50; integer in pcase sometimes compared by eq
Date: Sat, 16 Mar 2019 20:09:34 +0100

I can't find any similar problem elsewhere in the pcase code; singular bignum comparison was taken care of long ago.
Is the patch acceptable?

Message #16 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 34781 <at> debbugs.gnu.org
Subject: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 11:25:55 -0700

> I can't find any similar problem elsewhere in the pcase code; singular bignum comparison was taken care of long ago.
> Is the patch acceptable?

Thanks, it looks good to me; please install into the master branch

What happens if a source file that uses pcase is compiled on a 64-bit
machine that has wide fixnums, and is then loaded and run on a 32-bit
machine that has narrow fixnums? Will this pcase code still work? And if
not, are there similar bugs elsewhere in the pcase code?

Message #19 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 20:43:15 +0100

Mattias Engdegård <mattiase <at> acm.org> writes:

> (defun f (x)
>   (pcase x
>     ((or 'a #x10000000000000000) t)))
>
> (f #x10000000000000000) => nil
>
> Just replacing integerp with fixnump fixes this, but I'm not sure if
> more of the same lurks somewhere.

I had a quick look and didn't find anything obvious.

Michael.

Message #22 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 20:47:42 +0100

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> What happens if a source file that uses pcase is compiled on a 64-bit
> machine that has wide fixnums, and is then loaded and run on a 32-bit
> machine that has narrow fixnums? Will this pcase code still work? And
> if not, are there similar bugs elsewhere in the pcase code?

AFAIU the patch only corrects an optimization (use `memq' instead of
`member') that doesn't work for bignums.  This can never be harmful.

I had a quick look at pcase.el, and also tried the obvious cases (quote,
backquote), and didn't find any other obvious bug.


Michael.

Message #25 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 20:51:40 +0100

tor 2019-03-28 klockan 11:25 -0700 skrev Paul Eggert:
> 
> Thanks, it looks good to me; please install into the master branch
> 
> What happens if a source file that uses pcase is compiled on a 64-bit
> machine that has wide fixnums, and is then loaded and run on a 32-bit
> machine that has narrow fixnums? Will this pcase code still work? And
> if
> not, are there similar bugs elsewhere in the pcase code?

Not that I can see; the singular case uses eql for integerp.
I didn't think of the case you described, thanks. We then need a new
function:

  portable-fixnum-p
  guaranteed-fixnum-p
  always-fixnum-p
  fixnum-everywhere-p
  here-a-fixnum-there-a-fixnum-everywhere-a-fixnum-p

and names for the bounds:

  portable-most-{positive,negative}-fixnum
  ...

Name suggestions welcome. Meanwhile, I'll make a new patch.

Message #28 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 13:30:18 -0700

On 3/28/19 12:51 PM, Mattias Engdegård wrote:
> Not that I can see; the singular case uses eql for integerp.
> I didn't think of the case you described, thanks. We then need a new
> function:
>
>   portable-fixnum-p
> ...
>
> and names for the bounds:
>
>   portable-most-{positive,negative}-fixnum

If we have the bounds, then portable-fixnum-p is merely a convenience, no?

I'd prefer the names most-negative-portable-fixnum and
most-positive-portable-fixnum. Their documentation should make it clear
what the portability test is for. Presumably the test applies just to
this version of Emacs, since future versions might change the portable
fixnum bounds.

Message #31 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 13:33:45 -0700

On 3/28/19 12:47 PM, Michael Heerdegen wrote:
> AFAIU the patch only corrects an optimization (use `memq' instead of
> `member') that doesn't work for bignums.  This can never be harmful.

Isn't it harmful if a 64-bit Emacs decides that the optimization is safe
for the fixnum 1000000000 and thus generates the faster code, but the
code is put into an .elc file and then loaded by a 32-bit emacs that
treats 1000000000 as a bignum?

(An alternative to this annoying most-positive-portable-bignum business
would be to require --with-wide-int on all platforms. :-)

Message #34 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 22:30:33 +0100

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> On 3/28/19 12:47 PM, Michael Heerdegen wrote:
> > AFAIU the patch only corrects an optimization (use `memq' instead of
> > `member') that doesn't work for bignums.  This can never be harmful.
>
> Isn't it harmful if a 64-bit Emacs decides that the optimization is safe
> for the fixnum 1000000000 and thus generates the faster code, but the
> code is put into an .elc file and then loaded by a 32-bit emacs that
> treats 1000000000 as a bignum?

Ah ok - yes, I guess that would indeed happen.

Michael.

Message #37 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 22:51:39 +0100

[Message part 1 (text/plain, inline)]

tor 2019-03-28 klockan 13:30 -0700 skrev Paul Eggert:

> If we have the bounds, then portable-fixnum-p is merely a
> convenience, no?

Yes. We can drop it if you prefer it to be open-coded in pcase and
elsewhere, but I thought the predicate would make sense.

> I'd prefer the names most-negative-portable-fixnum and
> most-positive-portable-fixnum. Their documentation should make it
> clear
> what the portability test is for. Presumably the test applies just to
> this version of Emacs, since future versions might change the
> portable
> fixnum bounds.

Here is a patch for that, and an updated pcase patch.

[0001-Add-bounds-for-portable-fixnums-and-portable-fixnum-.patch (text/x-patch, attachment)]

[0002-Don-t-match-integers-with-memq-in-pcase.patch (text/x-patch, attachment)]

Message #40 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 15:10:14 -0700

>
> +Since the size of fixnums varies between platforms, the new predicate
> +'portable-fixnum-p' can be used to determine whether a number is
> +a fixnum on any machine running the current Emacs version.

The news item should also mention most-negative-portable-fixnum etc. Try
to be terser; e.g., "can be used to determine" -> "determines".


>
> +(defun portable-fixnum-p (object)
> +  "Return t if OBJECT is a fixnum on any machine running the current
> +Emacs version."

The usage message can fit on one line.

> +  (and (integerp object)
> +       (<= most-negative-portable-fixnum object
> most-positive-portable-fixnum)))

integerp -> fixnump


> +  DEFVAR_LISP ("most-positive-portable-fixnum",
> +               Vmost_positive_portable_fixnum,
> +               doc: /* The greatest integer that is represented
> efficiently
> +on any machine running this version of Emacs.

Try to have the first line explain things tersely. Something like "The
largest integer representable as a fixnum on any platform." More details
can be in later lines, if needed.

> +  Vmost_positive_portable_fixnum =
> make_fixnum(MOST_POSITIVE_PORTABLE_FIXNUM);

Space before parenthesis (elsewhere, too).


>
> diff --git a/src/lisp.h b/src/lisp.h
> index 178eebed2a..bf1f0a0bf5 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h

These changes should be in data.c not lisp.h, since only data.c needs
them and it's not likely any other code will need them.


>
> +/* The smallest portable value of EMACS_INT_MAX.  */
> +#define LEAST_EMACS_INT_MAX 2147483647   /* 2**31 - 1 */

There's no need to make it a macro. Also, the LEAST_* prefix and *_MAX
suffix are confusing: which takes priority? I suggest sticking to
suffixes, since that seems to be the convention. Something like

  int EMACS_INT_MAX_MIN = 2147483647;

as a local in the only function that needs it, and similarly for the
related macros. Although you can use 'verify' to check that
EMACS_INT_MAX_MIN <= EMACS_INT_MAX, I'm not sure I'd bother as we're
going to add overflow checking to make_fixnum at some point anyway.

This stuff should be documented in the manual, too, next to the
documentation of most-positive-fixnum and fixnump respectively.

Thanks again for taking this on.

Message #43 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 18:11:23 -0400

>                     (let ((upat (cddr alt)))
>                       (eq (car-safe upat) 'quote)))
>                (let ((val (cadr (cddr alt))))
> -                (unless (or (integerp val) (symbolp val))
> +                (unless (or (portable-fixnum-p val) (symbolp val))
>                    (setq memq-ok nil))
>                  (push (cadr (cddr alt)) simples))
>              (push alt others))))

Really?
I think the better option is below (since I think we should generally
move away from `eq` and replace it with `eql`).

Actually, the hunk below should have been installed at the same time
I replaced `eq` with `eql` when testing against an integer.  It was
a mere oversight.


        Stefan


diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index 9de240154..d9a20b1ff 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -802,7 +802,7 @@ pcase--u1
        ((> (length simples) 1)
         (pcase--u1 (cons `(match ,var
                                  . (pred (pcase--flip
-                                          ,(if memq-ok #'memq #'member)
+                                          ,(if memq-ok #'memql #'member)
                                           ',simples)))
                          (cdr matches))
                    code vars

Message #46 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 23:20:43 +0100

28 mars 2019 kl. 23.11 skrev Stefan Monnier <monnier <at> IRO.UMontreal.CA>:

> Actually, the hunk below should have been installed at the same time
> I replaced `eq` with `eql` when testing against an integer.  It was
> a mere oversight.

And all this work just because I didn't know we had memql. Serves me right.

Thank you Stefan, I'll do the obvious (unless you beat me to it).

I can still think of cases when portable-fixnum-p would be useful. I can clean up the patch (according to Paul's comments), or just sit on it.

Message #49 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Mattias Engdegård <mattiase <at> acm.org>,
 Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 34781 <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Thu, 28 Mar 2019 15:38:47 -0700

On 3/28/19 3:20 PM, Mattias Engdegård wrote:
> I'll do the obvious (unless you beat me to it).

When you do that, please also change the name of the local from memq-ok
to memql-ok, for clarity. Thanks.

Message #54 received at 34781-done <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 34781-done <at> debbugs.gnu.org
Subject: Re: 27.0.50; integer in pcase sometimes compared by eq
Date: Fri, 29 Mar 2019 00:03:01 +0100

[Message part 1 (text/plain, inline)]

tor 2019-03-28 klockan 15:38 -0700 skrev Paul Eggert:
> On 3/28/19 3:20 PM, Mattias Engdegård wrote:
> > I'll do the obvious (unless you beat me to it).
> 
> When you do that, please also change the name of the local from memq-
> ok
> to memql-ok, for clarity. Thanks.

Done. Thanks for your help! I'm attaching the mostly cleaned-up patch
(minus the necessary doc changes), in case someone will see some use
for it.

[0001-Add-bounds-for-portable-fixnums-and-portable-fixnum-.patch (text/x-patch, attachment)]

Message #57 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: eggert <at> cs.ucla.edu, monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Fri, 29 Mar 2019 11:48:07 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Thu, 28 Mar 2019 22:51:39 +0100
> Cc: monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org

In addition to Paul's comments:

> +Since the size of fixnums varies between platforms, the new predicate
> +'portable-fixnum-p' can be used to determine whether a number is
> +a fixnum on any machine running the current Emacs version.

This entry lacks a header.  NEWS is viewed in Outline mode, so it
should have headers that start with one or more '*' characters.
Please add a header for this item, and please make it short enough to
fit on a single line.

Also, it is advisable to accompany user-visible changes with suitable
changes in documentation, in this case the ELisp manual.  If you do
provide patches for the manuals, the NEWS entry should be marked with
"+++" to indicate that all the documentation will have been updated
when the patch is pushed.

> +(defun portable-fixnum-p (object)
> +  "Return t if OBJECT is a fixnum on any machine running the current
> +Emacs version."

Suggest to rephrase:

    "Non-nil if OBJECT is a fixnum on any platform.
  The value will be nil if OBJECT is not a number, or if its value
  needs more bits than a fixnum can support on some platforms."

> +  DEFVAR_LISP ("most-positive-portable-fixnum",
> +               Vmost_positive_portable_fixnum,
> +               doc: /* The greatest integer that is represented efficiently
> +on any machine running this version of Emacs.

The first line of any doc string must not be a complete sentence.
This is because apropos commands only display the first line of the
doc string.  So I suggest to reword:

  The largest integer representable as fixnum on any platform.

> +  DEFVAR_LISP ("most-negative-portable-fixnum",
> +               Vmost_negative_portable_fixnum,
> +               doc: /* The least integer that is represented efficiently
> +on any machine running this version of Emacs.

Similarly here.

Thanks for working on this.

Message #60 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, monnier <at> iro.umontreal.ca,
 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Fri, 29 Mar 2019 10:52:28 +0100

29 mars 2019 kl. 09.48 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> In addition to Paul's comments:

Eli, thanks for your kind review. Since portable-fixnum-p turned out not to be necessary for this bug after all, I decided not to go ahead with the patch. If you think it is still valuable enough on its own, say so and I'll polish it according to your notes.

Message #63 received at 34781 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: eggert <at> cs.ucla.edu, monnier <at> iro.umontreal.ca, 34781 <at> debbugs.gnu.org
Subject: Re: bug#34781: 27.0.50; integer in pcase sometimes compared by eq
Date: Fri, 29 Mar 2019 15:33:49 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Fri, 29 Mar 2019 10:52:28 +0100
> Cc: Paul Eggert <eggert <at> cs.ucla.edu>, monnier <at> iro.umontreal.ca,
>         34781 <at> debbugs.gnu.org
> 
> 29 mars 2019 kl. 09.48 skrev Eli Zaretskii <eliz <at> gnu.org>:
> > 
> > In addition to Paul's comments:
> 
> Eli, thanks for your kind review. Since portable-fixnum-p turned out not to be necessary for this bug after all, I decided not to go ahead with the patch. If you think it is still valuable enough on its own, say so and I'll polish it according to your notes.

I do think that something like that would be useful in the long run,
yes.  Not terribly important, though, so if you have better uses for
your time, I won't push.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.