bug#36139: [PATCH] Make better use of the switch op in cond forms

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

18 juni 2019 kl. 21.19 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>> A single `cond' form can how be compiled to any number of switch ops,
>> interspersed with non-switch conditions in arbitrary ways.
> 
> It can also be compiled to a bunch of switch ops only, right?
> (e.g. if it starts with a switch on `x` and then is followed by
> a switch on `y`)

Correct, and good catch. I rephrased the commit message.

>> +    (and (> (length cases) 1)
> 
> I think this `1` deserves a comment (IIRC it's the number of cases
> above which using a switch is expected to be faster than a sequence of
> tests).

Agreed, but the condition comes from the existing code (bytecomp.el:4180) where the number isn't motivated further either. I just assumed it was chosen with at least some care.

The ability to include multi-value cases in the switch makes the condition a conservative choice: if it is a good decision for single-value cases, it is definitely valid for multiple values per case.

I added a comment stating the intent, but it's not a lot more than restating the Lisp in English.

>> +        ;; Since `byte-compile-body' might increase `byte-compile-depth'
>> +        ;; by 1, not preserving its value will cause it to potentially
>> +        ;; increase by one for every clause body compiled, causing
>> +        ;; depth/tag conflicts or violating asserts down the road.
>> +        ;; To make sure `byte-compile-body' itself doesn't violate this,
>> +        ;; we use `cl-assert'.
>> +        (byte-compile-body body byte-compile--for-effect)
>> +        (cl-assert (or (= byte-compile-depth init-depth)
>> +                       (= byte-compile-depth (1+ init-depth))))
> 
> IIRC the depth is altered depending on byte-compile--for-effect (if
> byte-compile--for-effect is non-nil when entering the function but nil
> afterwards, depth should be identical, and it should be increased by
> 1 otherwise), so we should be able to tighten this assertion to replace
> the `or` with an `if`.

I'll do that in a separate change then, because it seems to be orthogonal to my changes.
Brief experiments seem to indicate that the

        (byte-compile-body body byte-compile--for-effect)

call does not seem to alter byte-compile--for-effect, but it does increase depth by 1 iff byte-compile--for-effect is non-nil.

> Other than that, the patch looks fine to me.

Thanks for the review! Pushed to master.