Re: [PATCH] python doc: Rework Breakpoint.__init__ doc

[Eli Zaretskii <eliz at gnu dot org>]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Thu, 7 Dec 2017 16:23:19 -0500
> 
> I find the documentation of the gdb.Breakpoint constructor hard to read
> and not very informative, especially since we have added the new
> linespec parameters.  There are multiple problems (some are subjective):
> 
> - It's not clear that you should use either the spec string or the
>   explicit arguments, not both.
> - It's not clear what combination of parameters you can use.
> - The big block of text describing the arguments is hard to read.
> - Currently, it seems like the "spec" argument is mandatory, even though
>   it is not (if you use explicit linespec).
> - The square bracket nesting
> 
>     [arg1 [, arg2[, arg3]]]
> 
>   makes it seems like if you specify arg3, you must specify arg1 and
>   arg2 (it's not the case here).
> 
> This patch tries to address these problems.

Thanks.  A few comments below.

> +A breakpoint can be created using one of the two forms of the
> +@code{gdb.Breakpoint} constructor.  The first one accepts a @code{spec} string
> +similar to what one would pass to the @code{break} command

Please add here a cross-reference to where these specs are described.

Also, does "similar" mean there can be some deviations?  If so, they
should be described here.  If there are no deviations, please use
"like one would pass" instead of "similar to what one would pass".

> +create both breakpoints and watchpoints.  The second accepts separate Python
> +arguments similar to @ref{Explicit Locations} and can only be used to create
                                                ^
You need a comma here.  Some versions of makeinfo might complain if
you don't.

> +@defun Breakpoint.__init__ (spec @r{[}, type @r{][}, wp_class @r{][}, internal @r{][}, temporary @r{]})
> +Create a new breakpoint according to @var{spec}, which is a string naming the
> +location of a breakpoint, or an expression that defines a watchpoint. The
                                                                       ^^
Two spaces.

> +contents can be any location recognized by the @code{break} command or, in the
> +case of a watchpoint, by the @code{watch} command.

"Contents" when referring to a string is confusing.  How about

  The string should describe a location in a format recognized by the
  @code{break} command (@pxref{CROSS-REFERENCE HERE})...

> +The optional @var{type} argument denotes the breakpoint to create from the types
> +defined later in this chapter.  This argument can be either
> +@code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}; it defaults to
> +@code{gdb.BP_BREAKPOINT}.

TYPE does not denote the breakpoint, it specifies its type.  So
suggest to reword"

  The optional @var{type} argument specifies the type of the
  breakpoint to create, as defined below.

> +The optional @var{wp_class} argument defines the class of watchpoint to create,
> +if @var{type} is @code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not
>  provided, it is assumed to be a @code{gdb.WP_WRITE} class.

I'd reword the last sentence:

  If @var{wp_class} is omitted, it defaults to @code{gdb.WP_WRITE}.

> +@defun Breakpoint.__init__ (@r{[} source @r{][}, function @r{][}, label @r{][}, line @r{]}, @r{][} internal @r{][}, temporary @r{][})
> +Create a new explicit location breakpoint (@pxref{Explicit Locations})
> +according to the specifications contained in the key words @var{source},
> +@var{function}, @var{label} and @var{line}.

You have told above that there are two forms of the constructor, but
the reader has read quite a lot of text since then.  So a reminder is
a good idea:

  This second form of creating a new breakpoint specifies the explicit
  location using key words.  The new breakpoint will be created in the
  specified source file @var{source}, the specified @var{function},
  @var{label} and @var{line}.

Btw, didn't you want to describe which combinations of these keywords
are valid?  Or maybe you should add a cross-reference to where that is
described for the CLI commands.

Thanks again for improving the manual.