python doc: Rework Breakpoint.__init__ doc

  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.

gdb/doc/ChangeLog:

	* python.texi (Manipulating breakpoints using Python): Split doc
	of Breakpoint.__init__ in two, split text in multiple
	paragraphs, don't nest parameter square brackets.
---
 gdb/doc/python.texi | 61 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 23 deletions(-)
  

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

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
  

> Date: Fri, 08 Dec 2017 12:59:31 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org
> 
> We have a cross-reference for break and watch just above, is it 
> necessary to put them at every reference of those commands?

No, not if they are close enough to one another.

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

This doesn't work well in some formats supported by Texinfo, so IMO
it's best to avoid that.

> It would work great for 
> HTML, PDF and info (formats with links), but not so much for plain text 
> formats.

Actually, it works well for HTML, less well for Info, and not at all
for PDF (because there are no hyperlinks in the printed version, only
"See this and that").

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

Not necessarily, you can keep it if you think it's useful.

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

Hmmm... now I'm confused: "at the same time" and "separate forms"
sounds like a contradiction to me.  My understanding was that one
needs to choose which constructor to use: the first or the second, and
then produce the spec accordingly.  Isn't that so?

Thanks.
  

On 2017-12-08 02:44 PM, Eli Zaretskii wrote:
>> Date: Fri, 08 Dec 2017 12:59:31 -0500
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org
>>
>> We have a cross-reference for break and watch just above, is it 
>> necessary to put them at every reference of those commands?
> 
> No, not if they are close enough to one another.

Ok.

>> 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?
> 
> This doesn't work well in some formats supported by Texinfo, so IMO
> it's best to avoid that.
> 
>> It would work great for 
>> HTML, PDF and info (formats with links), but not so much for plain text 
>> formats.
> 
> Actually, it works well for HTML, less well for Info, and not at all
> for PDF (because there are no hyperlinks in the printed version, only
> "See this and that").

Ok.

>>>> +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...)?
> 
> Not necessarily, you can keep it if you think it's useful.

Ok, I don't think it's useful to say

 This argument can be either @code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}

since there are other acceptable values as well referring to the list below is better.
So I'll remove that, but keep

  It defaults to @code{gdb.BP_BREAKPOINT}.

>>> 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().
> 
> Hmmm... now I'm confused: "at the same time" and "separate forms"
> sounds like a contradiction to me.

I don't see the contradiction:

  - don't use spec and function/line/label/source at the same time
  - you should choose (exclusively) one form (the one that uses spec) or
    the other (the one that uses function/line/label/source)

>My understanding was that one
> needs to choose which constructor to use: the first or the second, and
> then produce the spec accordingly.  Isn't that so?

What do you mean, "produce the spec accordingly" ?

One indeed has to choose between the two forms.  You can do

  1.1 gdb.Breakpoint("main")
  1.2 gdb.Breakpoint(spec="main")
  1.3 gdb.Breakpoint(spec="-function main")

Those are all equivalent, and use the first form.  spec is parsed as a
CLI-style linespec.  If you want to use the second form, you can do:

  2.1 gdb.Breakpoint(function="main")

The arguments of the second form are all keyword arguments, meaning that the
only way to pass a value to them is by naming them (such as the function= in
example 2.1).

However, showing two different constructors in the documentation is not technically
accurate with how it is implemented in the Python-support code, because it is
not possible to have two constructors with different signatures in Python.
Instead, we have a single constructor with all the parameters of both forms,
and validate that the user didn't mix arguments from both forms.  But I thought
it was a good way of explaining how it was meant to be used.

Simon
  

> CC: <gdb-patches@sourceware.org>
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Fri, 8 Dec 2017 15:46:59 -0500
> 
>   - don't use spec and function/line/label/source at the same time
>   - you should choose (exclusively) one form (the one that uses spec) or
>     the other (the one that uses function/line/label/source)

My question was about the 2nd form only: which of the keywords can or
cannot be used together, and what is the semantics of their
combinations?  I suspect the rules are the same as with the CLI
command, so I think we should either tell that or add a
cross-reference to where that's described for CLI.

> However, showing two different constructors in the documentation is not technically
> accurate with how it is implemented in the Python-support code, because it is
> not possible to have two constructors with different signatures in Python.

That's okay, since the manual says "2 forms", not "2 constructors".
Right?
  

On 2017-12-09 03:20 AM, Eli Zaretskii wrote:
>>   - don't use spec and function/line/label/source at the same time
>>   - you should choose (exclusively) one form (the one that uses spec) or
>>     the other (the one that uses function/line/label/source)
> 
> My question was about the 2nd form only: which of the keywords can or
> cannot be used together, and what is the semantics of their
> combinations?  I suspect the rules are the same as with the CLI
> command, so I think we should either tell that or add a
> cross-reference to where that's described for CLI.

We already have a cross-reference to Explicit Locations, which is where
the meaning of these parameters is documented.

  This second form of creating a new breakpoint specifies the explicit location
  (see Explicit Locations) using key words. The new breakpoint will be created
  in the specified source file source, at the specified function, label and line.

Do you have any comments on v2?

Simon
  

> CC: <gdb-patches@sourceware.org>
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Tue, 12 Dec 2017 16:16:13 -0500
> 
> Do you have any comments on v2?

Did it change anything wrt the version I reviewed?
  

On 2017-12-12 10:35 PM, Eli Zaretskii wrote:
>> CC: <gdb-patches@sourceware.org>
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> Date: Tue, 12 Dec 2017 16:16:13 -0500
>>
>> Do you have any comments on v2?
> 
> Did it change anything wrt the version I reviewed?
> 

Yes, sorry I should have pointed out what's new in that version.  I essentially
addressed your comments.  I added some cross-references and integrated your
suggestions of wording.

Thanks,

Simon
  

> Cc: gdb-patches@sourceware.org
> From: Simon Marchi <simark@simark.ca>
> Date: Tue, 12 Dec 2017 23:54:02 -0500
> 
> On 2017-12-12 10:35 PM, Eli Zaretskii wrote:
> >> CC: <gdb-patches@sourceware.org>
> >> From: Simon Marchi <simon.marchi@ericsson.com>
> >> Date: Tue, 12 Dec 2017 16:16:13 -0500
> >>
> >> Do you have any comments on v2?
> > 
> > Did it change anything wrt the version I reviewed?
> > 
> 
> Yes, sorry I should have pointed out what's new in that version.  I essentially
> addressed your comments.  I added some cross-references and integrated your
> suggestions of wording.

Ah, okay.  Then my only comment is that you should use "keywords" as a
single word.

Thanks.
  

On 2017-12-13 11:07, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Simon Marchi <simark@simark.ca>
>> Date: Tue, 12 Dec 2017 23:54:02 -0500
>> 
>> On 2017-12-12 10:35 PM, Eli Zaretskii wrote:
>> >> CC: <gdb-patches@sourceware.org>
>> >> From: Simon Marchi <simon.marchi@ericsson.com>
>> >> Date: Tue, 12 Dec 2017 16:16:13 -0500
>> >>
>> >> Do you have any comments on v2?
>> >
>> > Did it change anything wrt the version I reviewed?
>> >
>> 
>> Yes, sorry I should have pointed out what's new in that version.  I 
>> essentially
>> addressed your comments.  I added some cross-references and integrated 
>> your
>> suggestions of wording.
> 
> Ah, okay.  Then my only comment is that you should use "keywords" as a
> single word.
> 
> Thanks.

Thanks, I pushed it with that fixed.

Simon