From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71825 invoked by alias); 8 Dec 2017 14:12:53 -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 71815 invoked by uid 89); 8 Dec 2017 14:12:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: eggs.gnu.org Received: from eggs.gnu.org (HELO eggs.gnu.org) (208.118.235.92) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Dec 2017 14:12:51 +0000 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eNJOO-0001El-Fc for gdb-patches@sourceware.org; Fri, 08 Dec 2017 09:12:49 -0500 Received: from fencepost.gnu.org ([2001:4830:134:3::e]:39403) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eNJOO-0001Ef-9x; Fri, 08 Dec 2017 09:12:44 -0500 Received: from [176.228.60.248] (port=3120 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1eNJON-0003zg-1R; Fri, 08 Dec 2017 09:12:44 -0500 Date: Fri, 08 Dec 2017 14:12:00 -0000 Message-Id: <83fu8lwbpa.fsf@gnu.org> From: Eli Zaretskii To: Simon Marchi CC: gdb-patches@sourceware.org In-reply-to: <1512681799-12535-1-git-send-email-simon.marchi@ericsson.com> (message from Simon Marchi on Thu, 7 Dec 2017 16:23:19 -0500) Subject: Re: [PATCH] python doc: Rework Breakpoint.__init__ doc Reply-to: Eli Zaretskii References: <1512681799-12535-1-git-send-email-simon.marchi@ericsson.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00184.txt.bz2 > 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. 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.