unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
To: "Mattias Engdegård" <mattiase@acm.org>
Cc: Michael Heerdegen <michael_heerdegen@web.de>,
	Paul Pogonyshev <pogonyshev@gmail.com>,
	51982@debbugs.gnu.org
Subject: bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas
Date: Wed, 01 Dec 2021 13:34:01 -0500	[thread overview]
Message-ID: <jwvr1awyyuo.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <AF81C637-59D9-468B-917F-65AC37B13C75@acm.org> ("Mattias Engdegård"'s message of "Wed, 1 Dec 2021 17:04:44 +0100")

>> BTW, have you checked the impact on byte-code quality?
> With respect to these patches?

No I meant w.r.t removing `let*` (or `let` as the case may be).

> Yes: the B patch gives slightly better code because materialising the
> accessor (internal-get-closed-var N) is as cheap or cheaper than even
> a stack variable access. But the difference is small and since the
> case is rare it's probably insignificant.

I'm not worried at all about the performance of this corner-case.

>> Also, If mapping is of the form (car-safe SYMBOL) is `var` really the
>> correct answer?  Shouldn't it still be (cadr mapping)?
> Can there ever be a difference?

There's a big philosophical difference, yes.

>> Side note: I don't understand why we `(cons closedsym`, since that
>> `closedsym` can never appear in another binding (since it's fresh).
> Maybe it's to satisfy the invariant checked by the assertion at the top?

I don't think so because this one just checks that the
`cconv--remap-llv` was called where needed and did its job.

... [ goes and removes that `(cons closedsym` ] ...
... [ `make` ] ...

Oh, you're right!

>> I don't much like this `symbolp` test (which fundamentally seems to
>> be trying to recover the information about which branch of the `pcase`
>> we're coming from in `cconv--lifted-arg`).
> That's precisely what it is trying to do and no, I don't like it much either.

I think another way to do the patch B would be to replace `var` with
`lifted` right when we construct the (apply-partially ...) thingy
(i.e. in the :lambda-candidate part of the function), so those vars that
get remapped to `internal-get-closed-var` wouldn't even make their way
to `extend`.

> I don't see any alternative that is obviously better so I'm applying patch
> A. We can still go with B later on if we want; the changes are minor.

Good.

> Good comments, thank you very much!

[ I resent this implicit suggestion that I could ever write something less
  than a good comment.  ]


        Stefan






  reply	other threads:[~2021-12-01 18:34 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-19 20:31 bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas Paul Pogonyshev
2021-11-20  4:44 ` Michael Heerdegen
2021-11-20  8:45   ` Mattias Engdegård
2021-11-20 10:51     ` Michael Heerdegen
2021-11-20 16:54   ` Paul Pogonyshev
2021-11-20 17:04     ` Mattias Engdegård
2021-11-20 17:22       ` Paul Pogonyshev
2021-11-20 18:34         ` Mattias Engdegård
2021-11-20 20:53           ` Paul Pogonyshev
2021-11-21  7:59         ` Michael Heerdegen
2021-11-21  9:59           ` Mattias Engdegård
2021-11-22 10:29             ` Michael Heerdegen
2021-11-22 13:56               ` Mattias Engdegård
2021-11-22 17:35                 ` Mattias Engdegård
2021-11-30 14:12                   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-11-30 17:01                     ` Mattias Engdegård
2021-11-30 22:41                       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-12-01 16:04                         ` Mattias Engdegård
2021-12-01 18:34                           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors [this message]
2021-12-01 22:32                             ` Mattias Engdegård
2021-12-02  9:13                               ` Mattias Engdegård
2022-09-09 17:59                                 ` Lars Ingebrigtsen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwvr1awyyuo.fsf-monnier+emacs@gnu.org \
    --to=bug-gnu-emacs@gnu.org \
    --cc=51982@debbugs.gnu.org \
    --cc=mattiase@acm.org \
    --cc=michael_heerdegen@web.de \
    --cc=monnier@iro.umontreal.ca \
    --cc=pogonyshev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).