> On Mar 5, 2020, at 4:14 AM, martin rudalics <rudalics@gmx.at> wrote:
> 
> > Here is the patch.
> 
> Thanks. It gets me here the following warnings when building on master
> (sorry, I'm a complete ignorant of the 'cl-' snafu):
> 
>  ELC      progmodes/gdb-mi.elc
> 
> In toplevel form:
> ../../lisp/progmodes/gdb-mi.el:4738:1: Warning: Unused lexical variable
>    ‘window-config’
> 
> In end of data:
> ../../lisp/progmodes/gdb-mi.el:5069:1: Warning: the following functions might
>    not be defined at runtime: cl-delete-if, cl-find-if, cl-mapcar

Fixed.

> 
> A few remarks:
> 
> +  "If non-nil, gdb loads this window layout file on startup.
> +If not absolute, GDB will look under `gdb-window-layout-directory'."
> 
> We should settle in descriptions on whether we write gdb or GDB.  Also,
> "if not absolute" is too terse IMHO.
> 
> +  (define-key menu [load-layout] '("Load window layout" "Load GDB window layout from a file" . gdb-load-window-layout))
> +  (define-key menu [save-layout] '("Save window layout" "Save current GDB window layout to a file" . gdb-save-window-layout))
> 
> I think we could omit the "window" in these which should allow us to,
> instead of
> 
> +    '(menu-item "Restore window layout"
> 
> say
> 
> +    '(menu-item "Restore layout after quit"
> 
> so it becomes more clear that "load" and "save" act _immediately_ on the
> current state while "restore" is a more general setting that becomes
> effective only when the user quits.  (Note also that in general we
> cannot guarantee that menu tooltips are always shown on each and every
> system where Emacs runs.)

> 
> Also, I would mention all four possible values of
> 'gdb-restore-window-layout-after-quit' (currently "toggle" indicates
> that there are only two of them) like, for example, with the side values
> for tool-bar mode.

I removed “window” in their names. I mentioned the other possible values in the help echo, is that fine? Not sure where else I can put it.

> 
> +  "Return a buffer displaying source file or nil.
> +
> +The source file would be the most relevant file or the main file."
> 
> This is IMHO too terse in every respect.  Neither "source file" nor
> "main file" are canonical terms in the context of GDB so we should
> explain here how they are set up (what is nil in this context?).
> 
> +E.g., locals buffer, registers buffer, but don't include the main
> 
> I would say "Function buffers are locals buffers, ...".
> 
> +(defun gdb--buffer-type (buffer)
> +  "Return the buffer type of BUFFER or nil.
> 
> Maybe
> 
> "Return the type of BUFFER if it is a GDB function buffer."
> 
> would be better.

I added some explanations. I hope they are clear now.

> Thanks for your work on this, Martin

Thanks for reviewing :-)

Here is the new patch.

Yuan