bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Shigeru Fukaya]

Hello,

byte-compile of arithmatic comparison operators on more than two
arguments is incorrect.

(disassemble (lambda () (< (a) (b) (c))))

byte code:
  args: nil
0	constant  a
1	call	  0
2	constant  b
3	call	  0
4	lss	  
5	goto-if-nil-else-pop 1
8	constant  b
9	call	  0
10	constant  c
11	call	  0
12	lss	  
13:1	return	  


Functions a, b, c should be called once respectively if the function
has possibility of side effect.
But b may be called twice, and c may not be called.


I'm sorry for late report at the very time of release, but I noticed
just now.

Regards,
Shigeru

bug#18767: Acknowledgement (24.4; incorrect byte-compile on arithmatic comparisons)

[Shigeru Fukaya]

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

I tried to fix with the attached patch.

Regards,
Shigeru

[-- Attachment #2: bytecomp.diff --]
[-- Type: application/octet-stream, Size: 1254 bytes --]

--- bytecomp.el.orig	2014-03-21 14:34:40.000000000 +0900
+++ bytecomp.el	2014-10-19 19:17:00.915199400 +0900
@@ -3329,15 +3329,24 @@
     (byte-compile-form (nth 2 form))
     (byte-compile-out (get (car form) 'byte-opcode) 0)))
 
+(defun byte-compile-all-copyable-p (list)
+  "Non-nil if all elements of LIST satisfy `macroexp-copyable-p."
+  (while (and list (macroexp-copyable-p (car list)))
+    (setq list (cdr list)))
+  (null list))
+
 (defun byte-compile-and-folded (form)
   "Compile calls to functions like `<='.
-These implicitly `and' together a bunch of two-arg bytecodes."
+These implicitly `and' together a bunch of two-arg bytecodes when
+non-first operands are side-effect free."
   (let ((l (length form)))
     (cond
      ((< l 3) (byte-compile-form `(progn ,(nth 1 form) t)))
      ((= l 3) (byte-compile-two-args form))
-     (t (byte-compile-form `(and (,(car form) ,(nth 1 form) ,(nth 2 form))
-				 (,(car form) ,@(nthcdr 2 form))))))))
+     ((byte-compile-all-copyable-p (nthcdr 2 form))
+      (byte-compile-form `(and (,(car form) ,(nth 1 form) ,(nth 2 form))
+			       (,(car form) ,@(nthcdr 2 form)))))
+     (t (byte-compile-normal-call form)))))
 
 (defun byte-compile-three-args (form)
   (if (not (= (length form) 4))

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Stefan Monnier]

Version:24.5

> byte-compile of arithmatic comparison operators on more than two
> arguments is incorrect.

Indeed, I was not very awake when I committed that code.  I just
reverted the change so the byte-compiler simply doesn't optimize this case.
To do any better, I think we'll have to do something like:

loop over all args, doing "byte-compile-form" + "dup", and then apply
the comparisons backward, combining them with "and".


        Stefan

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Shigeru Fukaya]

I think your change of reversion will cause byte-compile error when
more than two args are specified.
Maybe you had better call byte-compile-normal-call for that case at least.

>loop over all args, doing "byte-compile-form" + "dup", and then apply
>the comparisons backward, combining them with "and".

Alternative is,

If non-first args are all constants or simple reference, fold them with AND,
otherwise call them at once by byte-compile-normal-call.


Shigeru


>Version:24.5
>
>> byte-compile of arithmatic comparison operators on more than two
>> arguments is incorrect.
>
>Indeed, I was not very awake when I committed that code.  I just
>reverted the change so the byte-compiler simply doesn't optimize this case.
>To do any better, I think we'll have to do something like:
>
>loop over all args, doing "byte-compile-form" + "dup", and then apply
>the comparisons backward, combining them with "and".
>
>
>        Stefan

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Stefan Monnier]

> I think your change of reversion will cause byte-compile error when
> more than two args are specified.

No errors, but yes, warnings.

>> loop over all args, doing "byte-compile-form" + "dup", and then apply
>> the comparisons backward, combining them with "and".
> Alternative is, If non-first args are all constants or simple
> reference, fold them with AND, otherwise call them at once by
> byte-compile-normal-call.

I guess that's OK, indeed.
I was worried that (<= 1 0 "a" nil) would return nil rather than
signal an error, but I see that this is already the case if you use
a normal call.  I installed your patch (except, using `cl-every').
Thank you,


        Stefan

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Shigeru Fukaya]

>I was worried that (<= 1 0 "a" nil) would return nil rather than
>signal an error, but I see that this is already the case if you use
>a normal call.  I installed your patch (except, using `cl-every').

How about adding the sentence to their help document, implying the
behavior, like,

  All args are evaluateed (without checking type) at first,
  then compared left to right.


Shigeru

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Stefan Monnier]

> How about adding the sentence to their help document, implying the
> behavior, like,
>   All args are evaluateed (without checking type)

This is necessarily true by virtue of being a function, so I don't think
we should repeat it.

> at first,
>   then compared left to right.

We could document that some of the later comparisons might be skipped, indeed.


        Stefan

bug#18767: 24.4; incorrect byte-compile on arithmatic comparisons

[Glenn Morris]

Stefan Monnier wrote:

> (except, using `cl-every').

This fails to bootstrap:

  simple.el:2490:32:Error: Symbol's function definition is void: cl-every