From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21945 invoked by alias); 8 Dec 2017 18:00:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 21935 invoked by uid 89); 8 Dec 2017 18:00:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=PDF X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Dec 2017 17:59:58 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id vB8HxpYH023631 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 8 Dec 2017 12:59:56 -0500 Received: by simark.ca (Postfix, from userid 112) id CF20E1E585; Fri, 8 Dec 2017 12:59:51 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 282731E02D; Fri, 8 Dec 2017 12:59:31 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 08 Dec 2017 18:00:00 -0000 From: Simon Marchi To: Eli Zaretskii Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] python doc: Rework Breakpoint.__init__ doc In-Reply-To: <83fu8lwbpa.fsf@gnu.org> References: <1512681799-12535-1-git-send-email-simon.marchi@ericsson.com> <83fu8lwbpa.fsf@gnu.org> Message-ID: <34fa897a804e9427dbe8ae3c107638e2@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.2 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 8 Dec 2017 17:59:51 +0000 X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00193.txt.bz2 On 2017-12-08 09:12, Eli Zaretskii wrote: >> From: Simon Marchi >> CC: Simon Marchi >> 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