* Re: [Emacs-diffs] feature/byte-unwind-protect 916094a 2/2: Add new bytecodes for unwind-protect [not found] ` <20180123051232.EBBB6207FC@vcs0.savannah.gnu.org> @ 2018-01-23 15:35 ` Stefan Monnier 2018-01-23 15:44 ` Tom Tromey 0 siblings, 1 reply; 3+ messages in thread From: Stefan Monnier @ 2018-01-23 15:35 UTC (permalink / raw) To: emacs-devel; +Cc: Tom Tromey > + (if (not byte-compile--use-old-handlers) > + (let ((except-tag (byte-compile-make-tag))) [...] > + (pcase (cddr form) > + (`(:fun-body ,f) > + (byte-compile-form > + (if byte-compile--use-old-handlers `(list (list 'funcall ,f)) f))) > + (handlers > + (if byte-compile--use-old-handlers Clearly the last two tests of byte-compile--use-old-handlers can be dropped since they're inside a branch where we know byte-compile--use-old-handlers is t. BTW, when introducing byte-codes we've "traditionally" done it in two steps: - in release N we add the code which handles the new byte-codes, and we add support for the new byte-code in the byte-compiler, but disabled by default. - in release N+1 we enable the use of the new byte-codes in the byte-compiler by default. This is done so as to reduce the pain when running a file compiled with another Emacs version. But we don't want to set byte-compile--use-old-handlers back to nil for your new bytecodes, so better use another variable. Also byte-compile--use-old-handlers can be retired, I think, because it has played its role. > + CASE (Bpushunwindprotect): /* New in 27.1. */ > + { > + struct handler *c = push_handler (Qt, CATCHER_ALL); Hmm... using this "catch+rethrow" method interferes with the debug-on-error stacktrace: (defun sm-test-1 () (unwind-protect (sm-test-2) (message "hello"))) (defun sm-test-2 () (message "test-1-in") (car 4)) (setq debug-on-error t) then call `sm-test-1` and you'll see that Emacs gives you a backtrace that gives the impression the error was signaled in `sm-test-1`! I also wonder if there can be places where we call something like `unbind_to` which should run those new unwind-forms but won't find it because it won't look at the handlers [ offhand, I can't think of any place where that would happen, and I think it's probably OK, but I can't quite convince myself that it definitely is. ] All in all, I'd really prefer if we kept the unwind forms in the specpdl. Was there a particular reason why you used the handlers instead? Stefan ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Emacs-diffs] feature/byte-unwind-protect 916094a 2/2: Add new bytecodes for unwind-protect 2018-01-23 15:35 ` [Emacs-diffs] feature/byte-unwind-protect 916094a 2/2: Add new bytecodes for unwind-protect Stefan Monnier @ 2018-01-23 15:44 ` Tom Tromey 2018-01-23 16:15 ` Stefan Monnier 0 siblings, 1 reply; 3+ messages in thread From: Tom Tromey @ 2018-01-23 15:44 UTC (permalink / raw) To: Stefan Monnier; +Cc: Tom Tromey, emacs-devel >>>>> "Stefan" == Stefan Monnier <monnier@IRO.UMontreal.CA> writes: >> + CASE (Bpushunwindprotect): /* New in 27.1. */ >> + { >> + struct handler *c = push_handler (Qt, CATCHER_ALL); Stefan> Hmm... using this "catch+rethrow" method interferes with the Stefan> debug-on-error stacktrace: Hmm, yeah. Any ideas for fixing this? One idea would be to capture the stack trace when CATCHER_ALL is targeted, then have a way to pass this to signal or throw. But often the stack trace isn't needed. Stefan> I also wonder if there can be places where we call something like Stefan> `unbind_to` which should run those new unwind-forms but won't find it Stefan> because it won't look at the handlers [ offhand, I can't think of any Stefan> place where that would happen, and I think it's probably OK, but Stefan> I can't quite convince myself that it definitely is. ] If this happens then surely it's already a problem for catch and condition-case? Stefan> All in all, I'd really prefer if we kept the unwind forms in Stefan> the specpdl. Was there a particular reason why you used the Stefan> handlers instead? This is how catch and condition-case are implemented. I do agree there's no reason to have a separate handler list. In fact this puzzled me for a while. However I think it's a separate cleanup. Tom ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Emacs-diffs] feature/byte-unwind-protect 916094a 2/2: Add new bytecodes for unwind-protect 2018-01-23 15:44 ` Tom Tromey @ 2018-01-23 16:15 ` Stefan Monnier 0 siblings, 0 replies; 3+ messages in thread From: Stefan Monnier @ 2018-01-23 16:15 UTC (permalink / raw) To: emacs-devel >>> + CASE (Bpushunwindprotect): /* New in 27.1. */ >>> + { >>> + struct handler *c = push_handler (Qt, CATCHER_ALL); > Stefan> Hmm... using this "catch+rethrow" method interferes with the > Stefan> debug-on-error stacktrace: > Hmm, yeah. > Any ideas for fixing this? Probably change the maybe_call_debugger code so it ignores CATCHER_ALL somehow. > One idea would be to capture the stack trace when CATCHER_ALL is > targeted, then have a way to pass this to signal or throw. In general, it'd be nice, yes, except that: > But often the stack trace isn't needed. Exactly. > Stefan> I also wonder if there can be places where we call something like > Stefan> `unbind_to` which should run those new unwind-forms but won't find it > Stefan> because it won't look at the handlers [ offhand, I can't think of any > Stefan> place where that would happen, and I think it's probably OK, but > Stefan> I can't quite convince myself that it definitely is. ] > If this happens then surely it's already a problem for catch and > condition-case? But having looked a bit more carefully at the code, I think we're OK, indeed. > Stefan> All in all, I'd really prefer if we kept the unwind forms in > Stefan> the specpdl. Was there a particular reason why you used the > Stefan> handlers instead? > This is how catch and condition-case are implemented. I know. But until now all the unwind forms are pushed to the specpdl, whereas the catcher and condition-case handlers go to the handlers stack. Your patch changes that by moving the unwind forms to the handlers stack (tho it keeps the "built-in unwinds" on the specpdl). > I do agree there's no reason to have a separate handler list. In fact > this puzzled me for a while. However I think it's a separate cleanup. Yes, merging the two is a different issue (last I looked at that code I was also tempted to go that way, but the two are used quite differently, so it's not clear there'd be much benefit). Stefan ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-23 16:15 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20180123051231.6240.43062@vcs0.savannah.gnu.org> [not found] ` <20180123051232.EBBB6207FC@vcs0.savannah.gnu.org> 2018-01-23 15:35 ` [Emacs-diffs] feature/byte-unwind-protect 916094a 2/2: Add new bytecodes for unwind-protect Stefan Monnier 2018-01-23 15:44 ` Tom Tromey 2018-01-23 16:15 ` Stefan Monnier
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.