Re: [PATCH] Add support to control auto-display behavior

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Fri, 6 Apr 2007 04:04:35 -0700
> From: Christopher Layne <clayne@anodized.com>
> 
> This patch is to allow setting how auto-displays are affected when memory is
> not accessible. It's very common to, when single-stepping, or stepping over,
> initially setup some auto-displays and then begin stepping. I don't see
> any reason why, in the event an object is not accessible, the auto-display
> should be disabled.

Thanks.  I agree that it's a good idea to have an option to control
this, unless we agree that what you want as an option should be how
GDB behaves in all cases.

I have a few comments for your patch.

> 	* printcmd.c (exceptions.h): Include.

A minor formatting issue: exceptions.h should not be in parentheses,
since it's not a function or macro.  Instead, use this:

	* printcmd.c: Include exceptions.h.

> +@cindex display options
> +@cindex display settings

It is better to have this as a single index entry, because they both
begin with the same string and both point to the same place in the
manual.  So having two of them is mostly redundant.  How about this
instead:

  @cindex display, optional settings

> +@value{GDBN} provides the following ways to control how auto-displays are
> +handled.

This sentence is better ended with a colon, don't you think?

> +@cindex disable/permit auto-displays on memory access error

When users look for such features in the manual, they don't think of
"disable" as the first word, I think.  They think about
"auto-display".  So I would change this index entry to say this
instead:

  @cindex auto-display, and memory access errors

> +@value{GDBN} automatically disables any auto-display which encounters a
> +memory access error during display. The default is @code{on}.
                                      ^
Two spaces after a period, please.

> +@item set display strict off
> +Do not disable any auto-display which encounters a memory access error,
> +but instead print a general error message rather than displaying a
> +non-accessible value.

How can one display a non-accessible value?

I'd rephrase the whole section as follows:

    +@table @code
    +@kindex set display
    +@item set display strict
    +@itemx set display strict on
    +@cindex disable/permit auto-displays on memory access error
    +When auto-displays is set to @samp{strict}, @value{GDBN}
    +automatically disables any auto-display that encounters a memory
    +access error during display. This is the default behavior.
    +
    +@item set display strict off
    +When strict display is turned off, @value{GDBN} prints an error
    +message, but does not disable an auto-display that encounters a memory
    +access error.

Btw, is it a good idea to call this ``strict'' display?  I wouldn't
characterize the current behavior as ``strict''.  How about "set
display disable-on-error on/off" instead?

> +  add_setshow_boolean_cmd ("strict", class_vars, 
> +			   &display_strict, _("\
> +Set disabling of display on memory access error."), _("\
> +Show disabling of display on memory access error."),

I'd prefer more help text here to explain in more detail what happens
under each setting.