unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#31718: 26.1; Strange behavior of `cond'
@ 2018-06-05  6:26 Ikumi Keita
  2018-06-05  8:07 ` Andreas Schwab
  2018-06-16 15:10 ` Paul Eggert
  0 siblings, 2 replies; 18+ messages in thread
From: Ikumi Keita @ 2018-06-05  6:26 UTC (permalink / raw)
  To: 31718

The special form `cond' does not return expected value under a specific
condition.

[How to reproduce]
1. Save the following file as /tmp/test.el
---- test.el ---------------------------------------------------------
(defun xyz (arg)
  "dummy"
;    (cond ((eq arg nil) ; OK
;    (cond ((eq arg 'abc) ; OK
;    (cond ((eq arg 'def) ; OK
    (cond ((eq arg 'default) ; NG
;    (cond ((eq arg 'default1) ; OK
	   "A")
	  (t
	   "B")))

(byte-compile 'xyz)

(insert (xyz t)) ; should "B", but "A" in actual
----------------------------------------------------------------------
2. emacs-26.1 -Q -l /tmp/test.el
3. Expected result:
   "B" is inserted in *scratch*
   Actual result:
   "A" is inserted in *scratch*

[Some notes related to this issue]
a. As the comments written in the above code, only the symbol `default'
   triggers this behavior.
b. Emacs 25.3 doesn't have this problem.
c. If `xyz' is not byte compiled, `cond' works as expected.
d. It seems that the following entry in NEWS is related:
** Certain cond/pcase/cl-case forms are now compiled using a faster jump
table implementation.  This uses a new bytecode op 'switch', which
isn't compatible with previous Emacs versions.  This functionality can
be disabled by setting 'byte-compile-cond-use-jump-table' to nil.
   If I replace the line `(byte-compile 'xyz)' with
(let ((byte-compile-cond-use-jump-table nil))
  (byte-compile 'xyz))
   in the above example, `cond' works as expected.


In GNU Emacs 26.1 (build 1, x86_64-unknown-freebsd11.1, GTK+ Version 3.22.29)
 of 2018-05-30 built on freebsd.vmware
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
Recent messages:
Saving file /home/keita/Mail/drafts/1...
Wrote /home/keita/Mail/drafts/1
next-line: End of buffer [2 times]
Saving file /home/keita/Mail/drafts/1...
Wrote /home/keita/Mail/drafts/1
next-line: End of buffer [4 times]
Sending...backgrounded
Scanning +drafts...done
No messages in +drafts, range (all)
Quit [2 times]
user-error: No cross-references in this node
Configured using:
 'configure --with-canna --with-canna-includes=/usr/local/canna/include
--with-canna-libraries=/usr/local/canna/lib --mandir=/usr/local/man
--without-xim --with-sound=yes --with-file-notification=yes
--disable-largefile --without-pop CFLAGS=-O3'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND DBUS GSETTINGS NOTIFY ACL GNUTLS
LIBXML2 FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 THREADS LCMS2

Important settings:
  value of $EMACSLOADPATH: /home/keita/elisp:
  value of $LANG: ja_JP.eucJP
  value of $XMODIFIERS: @im=fcitx
  locale-coding-system: japanese-iso-8bit-unix

Major mode: Info

Minor modes in effect:
  TeX-PDF-mode: t
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
~/elisp/preview hides /usr/local/share/emacs/site-lisp/auctex/preview
~/elisp/tex-buf hides /usr/local/share/emacs/site-lisp/auctex/tex-buf
~/elisp/tex hides /usr/local/share/emacs/site-lisp/auctex/tex
~/elisp/reftex-parse hides /usr/local/share/emacs/26.1/lisp/textmodes/reftex-parse

Features:
(shadow emacsbug latexenc supercite regi mh-identity mh-letter mh-comp
sendmail qp info view mh-alias multi-prompt smiley mm-archive mail-extr
mh-mime mh-gnus mh-show goto-addr gnus-cite gnus-art mm-uu mml2015
mm-view mml-smime smime dig mh-inc hl-line mh-tool-bar mh-seq mh-xface
mh-utils mh-folder which-func imenu mh-scan mh-e mh-compat mh-buffers
mh-loaddefs preview prv-emacs reftex-dcr reftex-auc reftex
reftex-loaddefs reftex-vars tex-fold tex-bar tex-buf toolbar-x
font-latex latex edmacro kmacro latex-flymake flymake-proc flymake
warnings thingatpt tex-ispell tex-style tex dbus crm tex-mode compile
shell misearch multi-isearch vc-dispatcher vc-hg org-archive skeleton
org-rmail org-mhe org-irc org-info org-gnus nnir gnus-sum gnus-group
gnus-undo gnus-start gnus-cloud nnimap nnmail mail-source utf7 netrc
nnoo gnus-spec gnus-int gnus-range message rfc822 mml mml-sec mm-decode
mm-bodies mm-encode mailabbrev gmm-utils mailheader gnus-win gnus
nnheader gnus-util rmail rmail-loaddefs mail-utils wid-edit org-docview
doc-view image-mode dired dired-loaddefs org-bibtex bibtex org-bbdb
org-w3m org-element avl-tree generator org org-macro org-footnote
org-pcomplete pcomplete org-list org-faces org-entities noutline outline
easy-mmode org-version ob-emacs-lisp ob ob-tangle org-src ob-ref ob-lob
ob-table ob-keys ob-exp ob-comint comint ansi-color ring ob-core ob-eval
org-compat org-macs org-loaddefs format-spec find-func cal-menu calendar
cal-loaddefs jka-compr cl-extra help-mode parse-time ucs-normalize json
map epa derived epg epg-config url-http tls gnutls url-auth mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr url-gw nsm rmc
puny seq twittering-mode easymenu advice url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
url-parse auth-source cl-seq eieio byte-opt bytecomp byte-compile cconv
eieio-core cl-macs gv eieio-loaddefs cl-loaddefs cl-lib password-cache
url-vars mailcap xml elec-pair w3m-load preview-latex auto-loads
tex-site canna-im time-date mule-util japan-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind kqueue lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 585780 62716)
 (symbols 48 43330 1)
 (miscs 40 1448 1041)
 (strings 32 118129 14954)
 (string-bytes 1 4152041)
 (vectors 16 71953)
 (vector-slots 8 1968759 71038)
 (floats 8 346 479)
 (intervals 56 6509 2189)
 (buffers 992 42))





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-05  6:26 bug#31718: 26.1; Strange behavior of `cond' Ikumi Keita
@ 2018-06-05  8:07 ` Andreas Schwab
  2018-06-06  5:41   ` Ikumi Keita
  2018-06-16 15:10 ` Paul Eggert
  1 sibling, 1 reply; 18+ messages in thread
From: Andreas Schwab @ 2018-06-05  8:07 UTC (permalink / raw)
  To: Ikumi Keita; +Cc: 31718, Vibhav Pant

On Jun 05 2018, Ikumi Keita <ikumi@ikumi.que.jp> wrote:

> (defun xyz (arg)
>   "dummy"
> ;    (cond ((eq arg nil) ; OK
> ;    (cond ((eq arg 'abc) ; OK
> ;    (cond ((eq arg 'def) ; OK
>     (cond ((eq arg 'default) ; NG

The byte-compiler uses 'default as a magic symbol, which breaks this
case.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-05  8:07 ` Andreas Schwab
@ 2018-06-06  5:41   ` Ikumi Keita
  2018-06-06  7:41     ` Robert Cochran
  0 siblings, 1 reply; 18+ messages in thread
From: Ikumi Keita @ 2018-06-06  5:41 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: 31718, Vibhav Pant

Hi Andreas, thanks for your reply.

>>>>> Andreas Schwab <schwab@suse.de> writes:
> On Jun 05 2018, Ikumi Keita <ikumi@ikumi.que.jp> wrote:
>> (defun xyz (arg)
>> "dummy"
>> ;    (cond ((eq arg nil) ; OK
>> ;    (cond ((eq arg 'abc) ; OK
>> ;    (cond ((eq arg 'def) ; OK
>> (cond ((eq arg 'default) ; NG

> The byte-compiler uses 'default as a magic symbol, which breaks this
> case.

Does this mean that this behavior is a (new) designed feature of elisp
and not a bug?
If so, is it the respoisibility of the authors of the codes to rewrite
not to use `default' or else to make sure to set
`byte-compile-cond-use-jump-table' to nil at byte compile?

Best regards,
Ikumi Keita





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-06  5:41   ` Ikumi Keita
@ 2018-06-06  7:41     ` Robert Cochran
  2018-06-06  9:14       ` Ikumi Keita
  0 siblings, 1 reply; 18+ messages in thread
From: Robert Cochran @ 2018-06-06  7:41 UTC (permalink / raw)
  To: Ikumi Keita; +Cc: Andreas Schwab, 31718, Vibhav Pant

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

Ikumi Keita <ikumi@ikumi.que.jp> writes:

> Hi Andreas, thanks for your reply.
>
>>>>>> Andreas Schwab <schwab@suse.de> writes:
>> On Jun 05 2018, Ikumi Keita <ikumi@ikumi.que.jp> wrote:
>>> (defun xyz (arg)
>>> "dummy"
>>> ;    (cond ((eq arg nil) ; OK
>>> ;    (cond ((eq arg 'abc) ; OK
>>> ;    (cond ((eq arg 'def) ; OK
>>> (cond ((eq arg 'default) ; NG
>
>> The byte-compiler uses 'default as a magic symbol, which breaks this
>> case.
>
> Does this mean that this behavior is a (new) designed feature of elisp
> and not a bug?
> If so, is it the respoisibility of the authors of the codes to rewrite
> not to use `default' or else to make sure to set
> `byte-compile-cond-use-jump-table' to nil at byte compile?

I for one consider this a bug, for 2 reasons:

1) It's not reasonable to expect a Lisp programmer to just know that
using the symbol default is problematic.

2) It creates diverging behavior between compiled and non-compiled Lisp.

To that end, I've made a small patch to rectify the behavior. Instead of
hardcoding a symbol, it uses gensym to create a unique one. I did a full
build of Emacs, as well as ran 'make check' and had identical results
pre- and post-change, so I'm reasonably sure it's correct.

Comments and corrections are of course welcomed.

HTH,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

-----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Use a gensym for byte compiler cond switch --]
[-- Type: text/x-patch, Size: 3118 bytes --]

From 4a025170b2b293810cf03c964b402963495fe7d7 Mon Sep 17 00:00:00 2001
From: Robert Cochran <robert-git@cochranmail.com>
Date: Wed, 6 Jun 2018 00:31:25 -0700
Subject: [PATCH] Use a gensym for the default case in
 byte-compile-cond-jump-table

* lisp/bytecomp.el (byte-compile-cond-jump-table): Create gensym to
  use as default case symbol
  (byte-compile-cond-jump-table-info): new argument `default-sym'; use
  it when generating default case clause
---
 lisp/emacs-lisp/bytecomp.el | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index ad6b5b7ce2..0fedfd0868 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4092,7 +4092,7 @@ byte-compile-cond-vars
    (and (symbolp obj1) (macroexp-const-p obj2) (cons obj1 obj2))
    (and (symbolp obj2) (macroexp-const-p obj1) (cons obj2 obj1))))
 
-(defun byte-compile-cond-jump-table-info (clauses)
+(defun byte-compile-cond-jump-table-info (clauses default-sym)
   "If CLAUSES is a `cond' form where:
 The condition for each clause is of the form (TEST VAR VALUE).
 VAR is a variable.
@@ -4124,14 +4124,15 @@ byte-compile-cond-jump-table-info
                         (not (assq obj2 cases)))
                    (push (list (if (consp obj2) (eval obj2) obj2) body) cases)
                  (if (and (macroexp-const-p condition) condition)
-                     (progn (push (list 'default (or body `(,condition))) cases)
+                     (progn (push (list default-sym (or body `(,condition))) cases)
                             (throw 'break t))
                    (setq ok nil)
                    (throw 'break nil))))))
          (list (cons prev-test prev-var) (nreverse cases)))))
 
 (defun byte-compile-cond-jump-table (clauses)
-  (let* ((table-info (byte-compile-cond-jump-table-info clauses))
+  (let* ((default-sym (gensym "byte-compile--cond-default-sym"))
+         (table-info (byte-compile-cond-jump-table-info clauses default-sym))
          (test (caar table-info))
          (var (cdar table-info))
          (cases (cadr table-info))
@@ -4141,7 +4142,7 @@ byte-compile-cond-jump-table
       ;; set it to `t' for cond forms with a small number of cases.
       (setq jump-table (make-hash-table :test test
                                         :purecopy t
-                                        :size (if (assq 'default cases)
+                                        :size (if (assq default-sym cases)
                                                   (1- (length cases))
                                                 (length cases)))
             default-tag (byte-compile-make-tag)
@@ -4175,8 +4176,8 @@ byte-compile-cond-jump-table
       (let ((byte-compile-depth byte-compile-depth))
         (byte-compile-goto 'byte-goto default-tag))
 
-      (when (assq 'default cases)
-        (setq default-case (cadr (assq 'default cases))
+      (when (assq default-sym cases)
+        (setq default-case (cadr (assq default-sym cases))
               cases (butlast cases 1)))
 
       (dolist (case cases)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-06  7:41     ` Robert Cochran
@ 2018-06-06  9:14       ` Ikumi Keita
  2018-06-12  1:34         ` Robert Cochran
  0 siblings, 1 reply; 18+ messages in thread
From: Ikumi Keita @ 2018-06-06  9:14 UTC (permalink / raw)
  To: Robert Cochran; +Cc: Andreas Schwab, 31718, Vibhav Pant

Hi Robert,

>>>>> Robert Cochran <robert-emacs@cochranmail.com> writes:
> Ikumi Keita <ikumi@ikumi.que.jp> writes:
>> Hi Andreas, thanks for your reply.
>> 
>>>>>>> Andreas Schwab <schwab@suse.de> writes:
>>> On Jun 05 2018, Ikumi Keita <ikumi@ikumi.que.jp> wrote:
>>>> (defun xyz (arg)
>>>> "dummy"
>>>> ;    (cond ((eq arg nil) ; OK
>>>> ;    (cond ((eq arg 'abc) ; OK
>>>> ;    (cond ((eq arg 'def) ; OK
>>>> (cond ((eq arg 'default) ; NG
>> 
>>> The byte-compiler uses 'default as a magic symbol, which breaks this
>>> case.
>> 
>> Does this mean that this behavior is a (new) designed feature of elisp
>> and not a bug?
>> If so, is it the respoisibility of the authors of the codes to rewrite
>> not to use `default' or else to make sure to set
>> `byte-compile-cond-use-jump-table' to nil at byte compile?

> I for one consider this a bug, for 2 reasons:

> 1) It's not reasonable to expect a Lisp programmer to just know that
> using the symbol default is problematic.

> 2) It creates diverging behavior between compiled and non-compiled Lisp.

I agree.

> To that end, I've made a small patch to rectify the behavior. Instead of
> hardcoding a symbol, it uses gensym to create a unique one. I did a full
> build of Emacs, as well as ran 'make check' and had identical results
> pre- and post-change, so I'm reasonably sure it's correct.

Thanks for the patch, it fixes the problem on my side!

Regards,
Ikumi Keita





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-06  9:14       ` Ikumi Keita
@ 2018-06-12  1:34         ` Robert Cochran
  2018-06-12 22:22           ` Noam Postavsky
  0 siblings, 1 reply; 18+ messages in thread
From: Robert Cochran @ 2018-06-12  1:34 UTC (permalink / raw)
  To: Ikumi Keita; +Cc: Andreas Schwab, 31718, Robert Cochran, Vibhav Pant

tags 31718 patch
quit

(I can do this as a mere luser, right? Moreover, did I DTRT?)

Ikumi Keita <ikumi@ikumi.que.jp> writes:

> Thanks for the patch, it fixes the problem on my side!

Good to hear! To that end, I'm going to (try to) mark the bug as 'patch
available'.

Also, thanks to Andreas for providing the key hint that made this change
much easier than would have been for me. Basically pointing me straight
to what I needed to look for was invaluable. :)

HTH, 
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-12  1:34         ` Robert Cochran
@ 2018-06-12 22:22           ` Noam Postavsky
  2018-06-13  5:51             ` Ikumi Keita
  0 siblings, 1 reply; 18+ messages in thread
From: Noam Postavsky @ 2018-06-12 22:22 UTC (permalink / raw)
  To: Robert Cochran; +Cc: Ikumi Keita, 31718, Vibhav Pant, Andreas Schwab

tags 31718 + patch
quit

Robert Cochran <robert-emacs@cochranmail.com> writes:

> tags 31718 patch
> quit
>
> (I can do this as a mere luser, right? Moreover, did I DTRT?)

Yes, and not quite.  You need to send those lines to
control@debbugs.gnu.org.  When doing it as part of a mailing list
thread, you should use Bcc to avoid having other people's replies also
go to control.  That's why do you don't see that address when I do it.
See https://debbugs.gnu.org/server-control.html for more info.

Anyway, the patch looks good, I think it should go to emacs-26 as it's
solving a pretty important regression.  I will push there in a couple of
days, assuming no objections.






^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-12 22:22           ` Noam Postavsky
@ 2018-06-13  5:51             ` Ikumi Keita
  2018-06-13  6:26               ` Vibhav Pant
  0 siblings, 1 reply; 18+ messages in thread
From: Ikumi Keita @ 2018-06-13  5:51 UTC (permalink / raw)
  To: Noam Postavsky, Robert Cochran; +Cc: Andreas Schwab, 31718, Vibhav Pant

Hi Noam and Robert,

>>>>> Noam Postavsky <npostavs@gmail.com> writes:

> Anyway, the patch looks good, I think it should go to emacs-26 as it's
> solving a pretty important regression.

That's a good news, thanks.

> I will push there in a couple of days, assuming no objections.

As a minor issue, I suppose the doc string of
`byte-compile-cond-jump-table-info' should include mention about a new
argument `default-sym'.

Regards,
Ikumi Keita





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-13  5:51             ` Ikumi Keita
@ 2018-06-13  6:26               ` Vibhav Pant
  0 siblings, 0 replies; 18+ messages in thread
From: Vibhav Pant @ 2018-06-13  6:26 UTC (permalink / raw)
  To: ikumi; +Cc: schwab, 31718, robert-emacs, npostavs

Hi,

Sorry for the late reply, this entire thread was somehow going into my
spam folder, and I didn't notice it until today.

The patch looks good to go, adding a doc-string for parameter
DEFAULT-SYM should be sufficient.

Thanks,
Vibhav

On Wed, Jun 13, 2018 at 11:21 AM Ikumi Keita <ikumi@ikumi.que.jp> wrote:
>
> Hi Noam and Robert,
>
> >>>>> Noam Postavsky <npostavs@gmail.com> writes:
>
> > Anyway, the patch looks good, I think it should go to emacs-26 as it's
> > solving a pretty important regression.
>
> That's a good news, thanks.
>
> > I will push there in a couple of days, assuming no objections.
>
> As a minor issue, I suppose the doc string of
> `byte-compile-cond-jump-table-info' should include mention about a new
> argument `default-sym'.
>
> Regards,
> Ikumi Keita



-- 
Vibhav Pant
vibhavp@gmail.com





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-05  6:26 bug#31718: 26.1; Strange behavior of `cond' Ikumi Keita
  2018-06-05  8:07 ` Andreas Schwab
@ 2018-06-16 15:10 ` Paul Eggert
  2018-06-16 15:20   ` Eli Zaretskii
                     ` (2 more replies)
  1 sibling, 3 replies; 18+ messages in thread
From: Paul Eggert @ 2018-06-16 15:10 UTC (permalink / raw)
  To: Robert Cochran
  Cc: Jay Kamat, Ikumi Keita, Héctor Enríquez Ramón,
	Noam Postavsky, Vibhav Pant, Basil L. Contovounesios, 31718,
	Pierre Téchoueyres, Andreas Schwab

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

Robert, thanks for your June 6 patch in Bug#31718#14. A small problem: gensym 
does not guarantee that the resulting symbol is unique, so the generated symbol 
could in theory appear in the input which would trigger the bug. Instead, let's 
use a guaranteed-unique object (cons nil nil). Also, I wrote a test case for 
this bug.

I installed the attached patch into the master branch. Given the practical 
consequences of this bug I expect the bug fix should be backported into the 
emacs-26 branch too.

[-- Attachment #2: 0001-Fix-byte-compilation-of-eq-foo-default.txt --]
[-- Type: text/plain, Size: 4182 bytes --]

From 9af399fd803ac1ca79f319945b9745b5b96122e7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@Penguin.CS.UCLA.EDU>
Date: Sat, 16 Jun 2018 07:44:58 -0700
Subject: [PATCH] Fix byte compilation of (eq foo 'default)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Do not use the symbol ‘default’ as a special marker.
Instead, use a value that cannot appear in the program,
improving on a patch proposed by Robert Cochran (Bug#31718#14).
* lisp/emacs-lisp/bytecomp.el (byte-compile--default-val):
New constant.
(byte-compile-cond-jump-table-info)
(byte-compile-cond-jump-table): Use it instead of 'default.
* test/lisp/emacs-lisp/bytecomp-tests.el:
(byte-opt-testsuite-arith-data): Add a test for the bug.
---
 lisp/emacs-lisp/bytecomp.el            | 24 +++++++++++++++---------
 test/lisp/emacs-lisp/bytecomp-tests.el |  9 ++++++++-
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index ad6b5b7..ee28e61 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4092,6 +4092,8 @@ byte-compile-cond-vars
    (and (symbolp obj1) (macroexp-const-p obj2) (cons obj1 obj2))
    (and (symbolp obj2) (macroexp-const-p obj1) (cons obj2 obj1))))
 
+(defconst byte-compile--default-val (cons nil nil) "A unique object.")
+
 (defun byte-compile-cond-jump-table-info (clauses)
   "If CLAUSES is a `cond' form where:
 The condition for each clause is of the form (TEST VAR VALUE).
@@ -4124,7 +4126,9 @@ byte-compile-cond-jump-table-info
                         (not (assq obj2 cases)))
                    (push (list (if (consp obj2) (eval obj2) obj2) body) cases)
                  (if (and (macroexp-const-p condition) condition)
-                     (progn (push (list 'default (or body `(,condition))) cases)
+		     (progn (push (list byte-compile--default-val
+					(or body `(,condition)))
+				  cases)
                             (throw 'break t))
                    (setq ok nil)
                    (throw 'break nil))))))
@@ -4139,11 +4143,12 @@ byte-compile-cond-jump-table
     (when (and cases (not (= (length cases) 1)))
       ;; TODO: Once :linear-search is implemented for `make-hash-table'
       ;; set it to `t' for cond forms with a small number of cases.
-      (setq jump-table (make-hash-table :test test
-                                        :purecopy t
-                                        :size (if (assq 'default cases)
-                                                  (1- (length cases))
-                                                (length cases)))
+      (setq jump-table (make-hash-table
+			:test test
+			:purecopy t
+			:size (if (assq byte-compile--default-val cases)
+				  (1- (length cases))
+				(length cases)))
             default-tag (byte-compile-make-tag)
             donetag (byte-compile-make-tag))
       ;; The structure of byte-switch code:
@@ -4175,9 +4180,10 @@ byte-compile-cond-jump-table
       (let ((byte-compile-depth byte-compile-depth))
         (byte-compile-goto 'byte-goto default-tag))
 
-      (when (assq 'default cases)
-        (setq default-case (cadr (assq 'default cases))
-              cases (butlast cases 1)))
+      (let ((default-match (assq byte-compile--default-val cases)))
+        (when default-match
+	  (setq default-case (cadr default-match)
+                cases (butlast cases))))
 
       (dolist (case cases)
         (setq tag (byte-compile-make-tag)
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index 7c5aa9a..ba62549 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -289,7 +289,14 @@ byte-opt-testsuite-arith-data
             (t)))
     (let ((a))
       (cond ((eq a 'foo) 'incorrect)
-            ('correct))))
+            ('correct)))
+    ;; Bug#31734
+    (let ((variable 0))
+      (cond
+       ((eq variable 'default)
+	(message "equal"))
+       (t
+	(message "not equal")))))
   "List of expression for test.
 Each element will be executed by interpreter and with
 bytecompiled code, and their results compared.")
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:10 ` Paul Eggert
@ 2018-06-16 15:20   ` Eli Zaretskii
  2018-06-16 15:28     ` Paul Eggert
  2018-06-16 15:23   ` Noam Postavsky
  2018-06-17  4:44   ` Michael Heerdegen
  2 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2018-06-16 15:20 UTC (permalink / raw)
  To: Paul Eggert
  Cc: robert-emacs, ikumi, hector.e.r, npostavs, vibhavp, contovob,
	jaygkamat, pierre.techoueyres, schwab, 31718

> Cc: 31718@debbugs.gnu.org, Ikumi Keita <ikumi@ikumi.que.jp>,
>  Vibhav Pant <vibhavp@gmail.com>, Noam Postavsky <npostavs@gmail.com>,
>  Andreas Schwab <schwab@suse.de>, Jay Kamat <jaygkamat@gmail.com>,
>  Eli Zaretskii <eliz@gnu.org>, Pierre Téchoueyres
>  <pierre.techoueyres@free.fr>, Héctor Enríquez Ramón
>  <hector.e.r@gmail.com>, "Basil L. Contovounesios" <contovob@tcd.ie>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 16 Jun 2018 08:10:02 -0700
> 
> Given the practical consequences of this bug I expect the bug fix
> should be backported into the emacs-26 branch too.

Could you explain why?  I'm not necessarily opposed, but AFAIU this
was Emacs's behavior since day one, so it isn't a recent regression.
Am I mistaken?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:10 ` Paul Eggert
  2018-06-16 15:20   ` Eli Zaretskii
@ 2018-06-16 15:23   ` Noam Postavsky
  2018-06-16 15:30     ` Paul Eggert
  2018-06-17  4:44   ` Michael Heerdegen
  2 siblings, 1 reply; 18+ messages in thread
From: Noam Postavsky @ 2018-06-16 15:23 UTC (permalink / raw)
  To: Paul Eggert
  Cc: Robert Cochran, Ikumi Keita, Pierre Téchoueyres, Vibhav Pant,
	Basil L. Contovounesios, Jay Kamat,
	Héctor Enríquez Ramón, Andreas Schwab, 31718

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

> A small problem: gensym does not guarantee that the resulting symbol
> is unique so the generated symbol could in theory appear in the input

What?

    gensym is a compiled Lisp function in ‘subr.el’.

    (gensym &optional PREFIX)

    Return a new uninterned symbol.
             ^^^

How could a new symbol have already appeared in the input?

(I have no objection to the alternative you used, just wondering why
gensym is not also correct)






^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:20   ` Eli Zaretskii
@ 2018-06-16 15:28     ` Paul Eggert
  2018-06-16 16:02       ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Paul Eggert @ 2018-06-16 15:28 UTC (permalink / raw)
  To: Eli Zaretskii
  Cc: robert-emacs, ikumi, hector.e.r, npostavs, vibhavp, contovob,
	jaygkamat, pierre.techoueyres, schwab, 31718

Eli Zaretskii wrote:
>> Given the practical consequences of this bug I expect the bug fix
>> should be backported into the emacs-26 branch too.
> Could you explain why?

We've had three independent reports of the bug (Bug#28806, Bug#31718, 
Bug#31734), I expect because org-mode triggers the Emacs bug. Bug#28806#5 says 
it occurs with Emacs 26 but not Emacs 25. Bug#31718#5 says it occurs with Emacs 
26 but not Emacs 25.3. So the bug is a regression, and it's biting real users.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:23   ` Noam Postavsky
@ 2018-06-16 15:30     ` Paul Eggert
  2018-06-16 23:00       ` Drew Adams
  0 siblings, 1 reply; 18+ messages in thread
From: Paul Eggert @ 2018-06-16 15:30 UTC (permalink / raw)
  To: Noam Postavsky
  Cc: Robert Cochran, Ikumi Keita, Pierre Téchoueyres, Vibhav Pant,
	Basil L. Contovounesios, Jay Kamat,
	Héctor Enríquez Ramón, Andreas Schwab, 31718

Noam Postavsky wrote:
>      Return a new uninterned symbol.

You're right, my mistake. Still, I prefer using (cons nil nil) since it's 
cheaper and is a common way to address this issue elsewhere.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:28     ` Paul Eggert
@ 2018-06-16 16:02       ` Eli Zaretskii
  2018-06-16 16:45         ` Paul Eggert
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2018-06-16 16:02 UTC (permalink / raw)
  To: Paul Eggert
  Cc: robert-emacs, ikumi, hector.e.r, npostavs, vibhavp, contovob,
	jaygkamat, pierre.techoueyres, schwab, 31718

> Cc: robert-emacs@cochranmail.com, 31718@debbugs.gnu.org, ikumi@ikumi.que.jp,
>  vibhavp@gmail.com, npostavs@gmail.com, schwab@suse.de, jaygkamat@gmail.com,
>  pierre.techoueyres@free.fr, hector.e.r@gmail.com, contovob@tcd.ie
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 16 Jun 2018 08:28:57 -0700
> 
> Eli Zaretskii wrote:
> >> Given the practical consequences of this bug I expect the bug fix
> >> should be backported into the emacs-26 branch too.
> > Could you explain why?
> 
> We've had three independent reports of the bug (Bug#28806, Bug#31718, 
> Bug#31734), I expect because org-mode triggers the Emacs bug. Bug#28806#5 says 
> it occurs with Emacs 26 but not Emacs 25. Bug#31718#5 says it occurs with Emacs 
> 26 but not Emacs 25.3. So the bug is a regression, and it's biting real users.

If it's a regression, then let's backport, indeed.

Thanks.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 16:02       ` Eli Zaretskii
@ 2018-06-16 16:45         ` Paul Eggert
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Eggert @ 2018-06-16 16:45 UTC (permalink / raw)
  To: Eli Zaretskii
  Cc: robert-emacs, 31718-done, ikumi, schwab, hector.e.r, npostavs,
	vibhavp, contovob, jaygkamat, pierre.techoueyres

Eli Zaretskii wrote:
> If it's a regression, then let's backport, indeed.

OK, done, and closing the bug report.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:30     ` Paul Eggert
@ 2018-06-16 23:00       ` Drew Adams
  0 siblings, 0 replies; 18+ messages in thread
From: Drew Adams @ 2018-06-16 23:00 UTC (permalink / raw)
  To: Paul Eggert, Noam Postavsky
  Cc: Robert Cochran, Ikumi Keita, Pierre Téchoueyres, Vibhav Pant,
	Basil L. Contovounesios, Jay Kamat,
	Héctor Enríquez Ramón, Andreas Schwab, 31718

> You're right, my mistake. Still, I prefer using (cons nil nil) since it's
> cheaper and is a common way to address this issue elsewhere.

Both ways are traditional, the cons-cell one being probably
more typical (and so more recognizable).

In any case, for such a (good, not bad) hack - either one,
a comment is always in order, I think.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#31718: 26.1; Strange behavior of `cond'
  2018-06-16 15:10 ` Paul Eggert
  2018-06-16 15:20   ` Eli Zaretskii
  2018-06-16 15:23   ` Noam Postavsky
@ 2018-06-17  4:44   ` Michael Heerdegen
  2 siblings, 0 replies; 18+ messages in thread
From: Michael Heerdegen @ 2018-06-17  4:44 UTC (permalink / raw)
  To: Paul Eggert
  Cc: Robert Cochran, Ikumi Keita, Héctor Enríquez Ramón,
	Noam Postavsky, Vibhav Pant, Basil L. Contovounesios, Jay Kamat,
	Pierre Téchoueyres, Andreas Schwab, 31718

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

> Robert, thanks for your June 6 patch in Bug#31718#14. A small problem:
> gensym does not guarantee that the resulting symbol is unique

Really?  AFAIK the name is not guaranteed to be unique, but the returned
symbol is.


Michael.





^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-06-17  4:44 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-05  6:26 bug#31718: 26.1; Strange behavior of `cond' Ikumi Keita
2018-06-05  8:07 ` Andreas Schwab
2018-06-06  5:41   ` Ikumi Keita
2018-06-06  7:41     ` Robert Cochran
2018-06-06  9:14       ` Ikumi Keita
2018-06-12  1:34         ` Robert Cochran
2018-06-12 22:22           ` Noam Postavsky
2018-06-13  5:51             ` Ikumi Keita
2018-06-13  6:26               ` Vibhav Pant
2018-06-16 15:10 ` Paul Eggert
2018-06-16 15:20   ` Eli Zaretskii
2018-06-16 15:28     ` Paul Eggert
2018-06-16 16:02       ` Eli Zaretskii
2018-06-16 16:45         ` Paul Eggert
2018-06-16 15:23   ` Noam Postavsky
2018-06-16 15:30     ` Paul Eggert
2018-06-16 23:00       ` Drew Adams
2018-06-17  4:44   ` Michael Heerdegen

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).