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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-12-08 09:12, Eli Zaretskii wrote:
> > 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.

Done, I also dropped the "@code{spec}" part.

> 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.

Done.

> > +@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.

Done.

> > +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})...

Done.

We have a cross-reference for break and watch just above, is it 
necessary to put them at every reference of those commands?

Alternatively, instead of adding a "(see Setting Breakpoints)", for 
example, is it possible to make the "break" word a link itself to the 
documentation of break?  I think it's less disruptive, and it's clear 
that it leads you to the doc of that command.  It would work great for 
HTML, PDF and info (formats with links), but not so much for plain text 
formats.  But then, I don't think that there is many people reading the 
doc in other formats than these three...

> > +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.

Done.  I understand that we get rid of the second sentence (This 
argument can be either...)?

> > +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}.

Done.

> > +@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}.

Sounds good, done.

> 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.

The important part is the fact that you can't use "spec" at the same 
time as source/line/label/function.  This should now be clear, because 
they are defined in the two separate forms of gdb.Breakpoint().  I don't 
want to go into much details about explicit locations here, but we could 
add a cross-reference to it in the previous paragraph that you 
suggested.

> Thanks again for improving the manual.

Thanks to you, I merely split what was already there, but your 
suggestions are very good.

Simon