Re: [PATCH] Add autoload-breakpoints [7/7] autoload-breakpoints doc

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Sat, 17 Mar 2012 16:53:19 +0800
> From: Hui Zhu <hui_zhu@mentor.com>
> CC: Stan Shebs <stan_shebs@mentor.com>
> 
> This patch add the doc for the autoload-breakpoints.

Thanks.

> +If the remote stub support, @value{GDBN} can get autoload-breakpoints from
> +remote stub.

 @value{GDBN} can receive the information about autoload-breakpoints
 from the remote stub, if the stub supports that.

Also, as we don't mention autoload-breakpoints anywhere else in the
manual, we need to explain here what these breakpoints are and why
controlling them is useful.

> +@item set breakpoint autoload query
> +If this option is query (the default), @value{GDBN} will query to user
                     ^^^^^
@samp{query}

> +how to handle the autoload-breakpints when @value{GDBN} connect to the stub.
                              ^^^^^^^^^^                   ^^^^^^^
A typo.  Also, "connects".

> +@item set breakpoint autoload merge
> +If this option is merge, the autoload-breakpoints of @value{GDBN}
                     ^^^^^
@samp{merge}

> +and stub will merge together when @value{GDBN} connect to stub.

I don't understand what it means to "merge" breakpoints.

> +@item set breakpoint autoload gdb
> +If this option is gdb, the autoload-breakpoints of stub will be removed
                     ^^^
@samp{gdb}

> +when GDB connect to stub.
        ^^^
@value{GDBN}

> +@item set breakpoint autoload stub
> +If this option is stub, the autoload-breakpoints of GDB will be removed
                     ^^^^                              ^^^
@samp{stub} and @value{GDBN}

> +@node Autoload-breakpoints Format
> +@section Autoload-breakpoints Format
> +@cindex Autoload-breakpoints Format
> +
> +@subsection Autoload-breakpoints base format

Please don't add subsections without a node (and a menu in the parent
section).  Such subsections cannot be navigated with on-line Info
commands.

> +Autoload-breakpoints base format describe the operation of
                                    ^^^^^^^^
"describes"

> +the autoload-breakpints in @value{GDBN} and the stub.
                ^^^^^^^^^^
A typo.

Anyway, it sounds like this describes the format of the packet, not of
autoload-breakpoints or something similar.  Therefore, I think it
should be part of the packet description, not a separate section and
not in this place in the manual.

> +@cindex @var{id}@samp{:}@var{command}@samp{:}@var{addr_string}@samp{:}@var{type}@samp{:}@var{ignore_num} packet

This index entry is useless; please remove it.

> +@table @samp
> +@item @var{id}
> +This is the id in hex string format of this command want to control.

 This is the number, as a hex string, of the autoload-breakpoint that
 this command wants to control.

> +@item @var{command}
> +This is command char. Include E (enable) and D (disable).

 This is the command character, either @samp{E} (for ``enable'') or
 @samp{D} (for ``disable'').

> +If this autoload-breakpoint @var{id} is not exist, create one and
> +enable or disable it.

 If the autoload-breakpoint @var{id} does not exist, create one and
 enable or disable it.

>                        If it is exist, item follow it will be ignore
> +and just disable and enable the autoload-breakpoints.

 If it does exist, the following items will be ignored, and the
 autoload-breakpoint will be enabled or disabled as specified by
 @var{command}.

> +@item @var{addr_string}
> +If need create an autoload-breakpoint, this is the address string
> +that encoded in hex string.

 This is the address of an autoload-breakpoint to create, encoded as a
 hex string.

> +@item @var{type}
> +If need create an autoload-breakpoint, this is the type in char
> +include H (hardware) and S (software).

 This is the type of the autoload-breakpoint to create, either
 @samp{H} (for ``hardware'') or @samp{S} (for ``software'').

> +@item @var{ignore_num}
> +If need create an autoload-breakpoint, this is the ignore_num in hex string.

 This is the ignore count of the autoload-breakpoint to create,
 encoded as a hex string.

> +@item @var{id}@samp{:}@samp{R}
> +@cindex @var{id}@samp{:}@samp{R} packet

Please remove this @cindex entry, it is useless.

> +@var{id} is the id in hex string format of this command want to remove.

 @var{id} is the number of the autoload-breakpoint that this command
 wants to remove, encoded as a hex string.

> +@item @var{id}@samp{:}@samp{C}@samp{:}@var{cmd_str}
> +@cindex @var{id}@samp{:}@samp{C}@samp{:}@var{cmd_str} packet

No need for this @cindex.

> +This packet add @var{cmd_str} to the commands list of
> +autoload-breakpoint @var{id}.

 This packet adds commands in @var{cmd_list} to the command list of
 the autoload-breakpoint whose number is @var{id}.

>                                 If @var{cmd_str} is empty,
> +clear commands list of autoload-breakpoint @var{id}.

 If @var{cmd_str} is empty, the command list will be emptied.

> +@var{cmd_str} is encoded to hex string.
                            ^^
"as"

> +@item @var{id}@samp{:}@samp{O}@samp{:}@var{condition_str}
> +@cindex @var{id}@samp{:}@samp{O}@samp{:}@var{condition_str} packet

No need for this @cindex entry.

> +This packet set @var{condition_str} as the condition of
> +autoload-breakpoint @var{id}.

 This packet sets the condition of the autoload-breakpoint @var{id} to
 be as specified by @var{condition_str}.

>                               If @var{condition_str} is empty,
> +clear condition of autoload-breakpoint @var{id}.

 If @var{condition_str} is empty, the autoload-breakpoint becomes
 unconditional.

> +@var{condition_str} is encoded to hex string.
                                  ^^
"as"

> +@subsection Autoload-breakpints request packets
> +@table @samp
> +@item qBfP
> +@itemx qBsP
> +These packets request data in autoload-breakpoints base format
> +about autoload-breakpints from the stub.
> +@value{GDBN} sends @code{qBfP} to get the first piece
> +of data, and multiple @code{qTsP} to get additional pieces.
> +@end table
> +
> +@subsection Autoload-breakpints control packets
> +@table @samp
> +@item @samp{QBDP}@var{autoload-breakpoints base format}
> +@value{GDBN} and the stub use this packet to control
> +the autoload-breakpoints in the remote.  The stub will translate
> +this packet through reportAsync Packets.
> +@end table

No need to have a separate subsection for each packet.

Also, I think you need to "connect the dots" by explaining the
connection between these packets and the format described above.  It
is probably a good idea to start with the list of the packets and
their short description, and then follow up with the description of
their format.

In sum, this documentation patch needs to be reworked.