[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#50245: 28.0.50; Instrumenting a function does not show "Edebug:" in
From: |
Stefan Monnier |
Subject: |
bug#50245: 28.0.50; Instrumenting a function does not show "Edebug:" in the echo area |
Date: |
Sat, 02 Oct 2021 11:06:48 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> bae2cfe63cbd11eaf348dfa7fbb2b9f7362fc747. I'm not sure, but I think
> that the author's intent was to reduce code duplication
Definitely.
> (with possibly other benefits).
This was a nasty case of duplication where the two versions worked quite
differently, yet trying to mimic the other's result. The worst part is
that depending on whether `edebug.el` was loaded either of the two codes
could be used, so any difference in behavior in the "normal C-M-x case"
was a bug, so the "mimicking" had to be as perfect as possible.
> My proposed patch first checks if edebugging is active and then avoids
> that eval-region prints by setting it's PRINTFLAG argument to nil:
>
> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
> index acf7123225..d1a4200df2 100644
> --- a/lisp/progmodes/elisp-mode.el
> +++ b/lisp/progmodes/elisp-mode.el
> @@ -1612,6 +1612,7 @@ elisp--eval-defun
> (let ((debug-on-error eval-expression-debug-on-error)
> (print-length eval-expression-print-length)
> (print-level eval-expression-print-level)
> + (edebugging edebug-all-defs)
> elisp--eval-defun-result)
> (save-excursion
> ;; Arrange for eval-region to "read" the (possibly) altered form.
I think you'll need a (defvar edebug-all-defs) before that.
> @@ -1629,8 +1630,9 @@ elisp--eval-defun
> ;; Alter the form if necessary.
> (let ((form (eval-sexp-add-defvars
> (elisp--eval-defun-1
> - (macroexpand form)))))
> - (eval-region beg end standard-output
> + (macroexpand form))))
> + (should-print (not edebugging)))
> + (eval-region beg end should-print
> (lambda (_ignore)
> ;; Skipping to the end of the specified region
> ;; will make eval-region return.
This replaces `standard-output` with t, which is probably OK in most
cases, but just to be sure, I'd use
(should-print (if (not edebugging) standard-output)))
> This solves the problem and does not introduce any further regression.
> Any feedback on if this is TRT?
This kind of dependency on Edebug is undesirable, but it's OK.
I can see 2 other approaches:
- Refrain from emitting the message if some message was emitted already
(i.e. checking `current-message`). This is likely brittle (and would
definitely break when `standard-output` points elsewhere ;-)
- Don't print here but print from within the `eval-region`, just like we do
in the Edebug case. Arguably this would be the cleanest in terms of
interaction between the Edebug and the non-Edebug case. But it likely
requires significantly more changes and might introduce more
problems elsewhere.
> If the patch looks good, I can accompany it with a comment that
> explains the reasoning, tests, etc.
Please do and thank you.
Stefan