GNU bug report logs - #36139
[PATCH] Make better use of the switch op in cond forms

Previous Next

Full log

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

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Wed, 19 Jun 2019 12:14:58 +0200

18 juni 2019 kl. 21.19 skrev Stefan Monnier <monnier <at> 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.

Previous Next

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