30 nov. 2021 kl. 15.12 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Can we avoid this duplication by moving that code to a separate function?

I extracted a big part of the code into a common function but left the free variable access and mutation outside. (Really want to get rid of `let*`!)

> These two tests are identical aren't they?

No, they exercise different code paths (let and let*).

>  Also, can we change the
> (setq x x) into something like (setq x (list x x)) and avoid using the
> same `b` value for both `x` vars, so as to catch more potential errors?

Yes, thank you, it was an editing mistake. Fixed.

> Looks good (better than patch A).

And here I was prepared to apply patch A since it's slightly more conservative and it seems to be a rare problem anyway.
I've now split the patches in a more sensible (and easily reviewed) way: the first corresponds to patch A, and the second is the diff to B. Take a second look before making up your mind.

> You say "On the other hand, patch B does abuse the cconv data structures
> a little (but it works!)" so the code should say something about
> this abuse.  A least I failed to see where the abuse lies.

There are comments and doc strings such as

  EXTEND is a list of variables which might need to be accessed even from places
  where they are shadowed, because some part of ENV causes them to be used at
  places where they originally did not directly appear.

but with the B patch we put things into `extend` that are not strictly variables but (international-get-closed-var N).
Similarly, `env` has entries like (VAR . (apply-partially F ARG1 ARG2 ..)) where the ARGi are always treated as variables but now they can be access forms as well.

I suppose it doesn't matter much. There is an assertion at the very top of `cconv-convert` which compares the elements by `eq` but it seems to work all right...

Thanks for the review – new patches attached.