bug#34781: 27.0.50; integer in pcase sometimes compared by eq

["Mattias Engdegård" <mattiase@acm.org>]

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

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.


[-- Attachment #2: 0001-Add-bounds-for-portable-fixnums-and-portable-fixnum-.patch --]
[-- Type: text/x-patch, Size: 4717 bytes --]

From f5fe5d987090e69b0f7438435f69c6aab4215e5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 28 Mar 2019 22:12:37 +0100
Subject: [PATCH 1/2] Add bounds for portable fixnums, and portable-fixnum-p

These are useful for macros that need to detect whether a number is a
fixnum on any machine, so that the bytecode becomes portable (Bug#34781).

* src/lisp.h (LEAST_EMACS_INT_MAX, MOST_POSITIVE_PORTABLE_FIXNUM,
MOST_NEGATIVE_PORTABLE_FIXNUM):
* src/data.c (most-positive-portable-fixnum, most-negative-portable-fixnum):
* lisp/subr.el (portable-fixnum-p):
New.
* etc/NEWS (Lisp Changes): Mention portable-fixnum-p.
---
 etc/NEWS     |  4 ++++
 lisp/subr.el |  6 ++++++
 src/data.c   | 18 ++++++++++++++++++
 src/lisp.h   | 12 ++++++++++++
 4 files changed, 40 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 7486d6bcfe..a2933fbbd3 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1424,6 +1424,10 @@ like 'file-attributes' that compute file sizes and other attributes,
 functions like 'process-id' that compute process IDs, and functions like
 'user-uid' and 'group-gid' that compute user and group IDs.
 
+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.
+
 +++
 ** Although the default timestamp format is still (HI LO US PS),
 it is planned to change in a future Emacs version, to exploit bignums.
diff --git a/lisp/subr.el b/lisp/subr.el
index f1a1dddd81..950a0b58e3 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -376,6 +376,12 @@ bignump
   "Return t if OBJECT is a bignum."
   (and (integerp object) (not (fixnump object))))
 
+(defun portable-fixnum-p (object)
+  "Return t if OBJECT is a fixnum on any machine running the current
+Emacs version."
+  (and (integerp object)
+       (<= most-negative-portable-fixnum object most-positive-portable-fixnum)))
+
 (defun lsh (value count)
   "Return VALUE with its bits shifted left by COUNT.
 If COUNT is negative, shifting is actually to the right.
diff --git a/src/data.c b/src/data.c
index 15b6106cfe..2969f2df82 100644
--- a/src/data.c
+++ b/src/data.c
@@ -4110,6 +4110,24 @@ This variable cannot be set; trying to do so will signal an error.  */);
   Vmost_negative_fixnum = make_fixnum (MOST_NEGATIVE_FIXNUM);
   make_symbol_constant (intern_c_string ("most-negative-fixnum"));
 
+  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.
+This variable can be used to ensure portability of bytecode that works
+with fixnums.  It cannot be set; trying to do so will signal an error.  */);
+  Vmost_positive_portable_fixnum = make_fixnum(MOST_POSITIVE_PORTABLE_FIXNUM);
+  make_symbol_constant (intern_c_string ("most-positive-portable-fixnum"));
+
+  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.
+This variable can be used to ensure portability of bytecode that works
+with fixnums.  It cannot be set; trying to do so will signal an error.  */);
+  Vmost_negative_portable_fixnum = make_fixnum(MOST_NEGATIVE_PORTABLE_FIXNUM);
+  make_symbol_constant (intern_c_string ("most-negative-portable-fixnum"));
+
   DEFSYM (Qwatchers, "watchers");
   DEFSYM (Qmakunbound, "makunbound");
   DEFSYM (Qunlet, "unlet");
diff --git a/src/lisp.h b/src/lisp.h
index 178eebed2a..bf1f0a0bf5 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -112,6 +112,13 @@ enum { EMACS_INT_WIDTH = LLONG_WIDTH, EMACS_UINT_WIDTH = ULLONG_WIDTH };
 # endif
 #endif
 
+/* The smallest portable value of EMACS_INT_MAX.  */
+#define LEAST_EMACS_INT_MAX 2147483647   /* 2**31 - 1 */
+
+#if EMACS_INT_MAX < LEAST_EMACS_INT_MAX
+# error "EMACS_INT_MAX less than LEAST_EMACS_INT_MAX"
+#endif
+
 /* Number of bits to put in each character in the internal representation
    of bool vectors.  This should not vary across implementations.  */
 enum {  BOOL_VECTOR_BITS_PER_CHAR =
@@ -1146,6 +1153,11 @@ enum More_Lisp_Bits
 #define MOST_POSITIVE_FIXNUM (EMACS_INT_MAX >> INTTYPEBITS)
 #define MOST_NEGATIVE_FIXNUM (-1 - MOST_POSITIVE_FIXNUM)
 
+/* Largest and smallest values that are guaranteed to be representable
+   as fixnums on any machine.  These are the C values.  */
+#define MOST_POSITIVE_PORTABLE_FIXNUM (LEAST_EMACS_INT_MAX >> INTTYPEBITS)
+#define MOST_NEGATIVE_PORTABLE_FIXNUM (-1 - MOST_POSITIVE_PORTABLE_FIXNUM)
+
 #if USE_LSB_TAG
 
 INLINE Lisp_Object
-- 
2.20.1


[-- Attachment #3: 0002-Don-t-match-integers-with-memq-in-pcase.patch --]
[-- Type: text/x-patch, Size: 2017 bytes --]

From 5054ac21b6bdb522437d97db2a514a53d8ce7773 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 12 Mar 2019 13:19:35 +0100
Subject: [PATCH 2/2] Don't match integers with `memq' in `pcase'

* lisp/emacs-lisp/pcase.el (pcase--u1):
Use portable-fixnum-p instead of integerp as criterion for memq (Bug#34781).
* test/lisp/emacs-lisp/pcase-tests.el (pcase-tests-member): Test the above.
---
 lisp/emacs-lisp/pcase.el            | 2 +-
 test/lisp/emacs-lisp/pcase-tests.el | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index 9de2401549..a13694ed33 100644
--- 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 (portable-fixnum-p val) (symbolp val))
                   (setq memq-ok nil))
                 (push (cadr (cddr alt)) simples))
             (push alt others))))
diff --git a/test/lisp/emacs-lisp/pcase-tests.el b/test/lisp/emacs-lisp/pcase-tests.el
index 1e9d37fbfa..29f02cbaa9 100644
--- a/test/lisp/emacs-lisp/pcase-tests.el
+++ b/test/lisp/emacs-lisp/pcase-tests.el
@@ -56,6 +56,12 @@ pcase-tests-grep
            'member (macroexpand-all '(pcase x ((or "a" 2 3) body)))))
   (should-not (pcase-tests-grep
                'memq (macroexpand-all '(pcase x ((or "a" 2 3) body)))))
+  (should (pcase-tests-grep
+           'member (macroexpand-all '(pcase x ((or #x100000000 2 3)
+                                               body)))))
+  (should-not (pcase-tests-grep
+               'memq (macroexpand-all '(pcase x ((or #x100000000 2 3)
+                                                 body)))))
   (let ((exp (macroexpand-all
                       '(pcase x
                          ("a" body1)
-- 
2.20.1