Re: debugger-toggle-locals

[Stefan Monnier]

>       x: 9
>       y: 114
>       z: (9 . 114)

Nitpick: I'd rather have "x=" instead of "x:" (for me, ":" means "has type").

> +(defun debugger--backtrace-base ()
> +  "Return the function name that marks the top of the backtrace.
> +See `backtrace-frame'."
> +  (cond ((eq 'debug--implement-debug-on-entry
> +          (cadr (backtrace-frame 1 'debug)))
> +      'debug--implement-debug-on-entry)
> +     (t 'debug)))

Please use it in debugger-eval-expression as well.

> +(defun debugger--locals-visible-p ()
> +  "Are the local variables of the current stack frame visible?"
> +  (save-excursion
> +    (move-to-column 2)
> +    (get-text-property (point) 'debugger-locals-visible-p)))

The text property shouldn't have a name that ends in `-p'; these names
are for predicates (i.e. functions returning a boolean), not for
variables or object fields, or symbol/text properties.

> +  (dolist (s+v locals)
> +    (let ((symbol (car s+v))
> +       (value (cdr s+v))

You can use (pcase-dolist (`(,symbol . ,value) locals) ...).

> +      (cond ((string= (symbol-name symbol) 
> "internal-interpreter-environment")
> +          (cond ((or (null value)
> +                     (and (null (cdr value))
> +                          (symbol (car value))))
                              ^^^^^^
                              should be `symbolp', right?

`value' can also be of the form (a b t).
I.e. (null (cdr value)) is false, but there are no values.

But I tend to think that the handling of
internal-interpreter-environment should be kept in the C code: this
symbol should by and large not be exported to Elisp.

> +  backtrace_eval_unrewind (distance);
[...]
> +  /* restore values from specpdl to orignal place */
> +  backtrace_eval_unrewind (-distance);

It's kind of annoying that we have to modify the backtrace in order to
build the result, since the function is otherwise side-effect-free.
It's admittedly simpler to do it as above rather than "doing it right".


        Stefan