Re: Extend gdb to filter registers

[Stefan Monnier]

I just took a quick look at your code.  Looks pretty good.
See below the few nitpicks I found.


        Stefan


> diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
> index 60852e4ad6..5b7e3321f3 100644
> --- a/lisp/progmodes/gdb-mi.el
> +++ b/lisp/progmodes/gdb-mi.el
> @@ -200,6 +200,13 @@ gdb-error
>  (defvar gdb-macro-info nil
>    "Non-nil if GDB knows that the inferior includes preprocessor macro info.")
>  (defvar gdb-register-names nil "List of register names.")
> +(defvar gdb-display-these-registers t
> +  "Registers that are displayed in register buffer.
> +Can be a list, a function or t/nil.
> +If a list, registers which names can be found in the list are displayed.
> +If a function, it is passed with a register name and should
> +return t to display and nil to not display.
> +If t/nil, display all registers / none of the registers.")

"which" => "whose"
Note: nil is a list, so the nil case is already covered by the list case.
For the function case, please clarify what the argument will look like
(a symbol or a string?).

Is this expected to be set by the user via `setq`?
If so, maybe it should be a `defcustom`?

> +(defun gdb-registers-add-to-display (register)
> +  "Add REGISTER to display in register buffer.
> +REGISTER is a string representing the name of the register.
> +By default the register buffer displays all the registers.
> +REGISTER can also be 'all, which configures gdb to display all registers.
> +Also see `gdb-registers-remove-from-display'. "
> +  (interactive (list (completing-read "Register: " (append gdb-register-names
> +                                                           '("all")))))
> +  (if (equal register "all")
> +      (setq gdb-display-these-registers t)
> +    (if (listp gdb-display-these-registers)
> +        (add-to-list 'gdb-display-these-registers register)
> +      (setq gdb-display-these-registers (list register)))))

If gdb-display-these-registers is t this will not "add" but remove all
but `register` from the display, which might surprise the user.
Along the same lines, if gdb-display-these-registers is a function this
will either silently throw away the function (if `listp` returns nil) or
build a "broken" list (if `listp` return non-nil, which is very much
possible).

> +(defun gdb-registers-remove-from-display (register)
> +  "Remove REGISTER from display in register buffer.
> +REGISTER is a string representing the name of the register.
> +By default the register buffer displays all the registers.
> +REGISTER can also be 'all, which configures gdb to hide all registers.
> +Also see `gdb-registers-add-to-display'."
> +  (interactive (list (completing-read "Register: " (append gdb-register-names
> +                                                           '("all")))))
> +  (if (equal register "all")
> +      (setq gdb-display-these-registers nil)
> +    (if (listp gdb-display-these-registers)
> +        (setq gdb-display-these-registers
> +              (remove register gdb-display-these-registers))
> +      (user-error "gdb-display-these-registers is not a list, can’t remove 
> anything from it"))))

This may similarly misbehave if gdb-display-these-registers is
a function represented as a list (tho most likely in that case
`register` won't be found in the list, so it will just silently do
nothing).

> +          (when (cond ((and (listp gdb-display-these-registers)
> +                            gdb-display-these-registers) ; not nil
> +                       (member register-name gdb-display-these-registers))

`consp` is a more efficient to test "list and non-nil".
But here, we don't need to avoid the nil case, since (member X nil) will
return nil anyway.


        Stefan