From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25408 invoked by alias); 29 Mar 2010 14:28:03 -0000 Received: (qmail 25385 invoked by uid 22791); 29 Mar 2010 14:28:01 -0000 X-SWARE-Spam-Status: No, hits=2.5 required=5.0 tests=BAYES_00,RCVD_IN_JMF_BL,RCVD_IN_SORBS_WEB,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 29 Mar 2010 14:27:54 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0L0100J00RK2YN00@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Mon, 29 Mar 2010 17:26:32 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.127.176.135]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0L0100ISVS42ND50@a-mtaout20.012.net.il>; Mon, 29 Mar 2010 17:26:27 +0300 (IDT) Date: Mon, 29 Mar 2010 14:28:00 -0000 From: Eli Zaretskii Subject: Re: [patch][python] Add breakpoint support. In-reply-to: <4BB0B063.6000600@redhat.com> To: Phil Muldoon Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83k4sv19wm.fsf@gnu.org> References: <4BB0B063.6000600@redhat.com> X-IsSubscribed: yes 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 X-SW-Source: 2010-03/txt/msg00987.txt.bz2 > Date: Mon, 29 Mar 2010 14:51:31 +0100 > From: Phil Muldoon > > This patch ports the Python breakpoint work found in > archer-tromey-python in the Archer git repository. In addition to > the port, this patch adds watchpoint support and several other > bug-fixes and clean-ups not found in the archer repository. It also > adds a testsuite for breakpoints. Ok? Thanks. I have a few comments to the documentation part. > * breakpoint.c (condition_command): Simplify. Move condition > setting code too ... ^^^ A typo. > +@tindex gdb.Breakpoint > +@tindex Breakpoint You already added index entries with the same names, albeit in different letter-case, elsewhere in the manual. Why is it a good idea to have these here as well? > +Create a new breakpoint. @var{expression} is a string naming the > +location of the breakpoint, or an expression that defines a > +watchpoint. This sounds like the watchpoint expression is not a string. Also, I wouldn't use "expression" here, since it isn't in the case of a breakpoint. How about this variant: @defmethod Breakpoint __init__ spec @r{[}type@r{]} @r{[}wp_class@r{]} ... @var{spec} specifies the location of a breakpoint or the expression to watch for a watchpoint. > +either: @var{BP_BREAKPOINT} or @var{BP_WATCHPOINT}. If BP_BREAKPOINT and BP_WATCHPOINT should be in @code, not in @var. They stand for themselves, not for something else (contrast with your @var{expression} above). > +If no type is provided, it is presumed to be @var{BP_BREAKPOINT}. "@var{type} defaults to @code{BP_BREAKPOINT}." is much shorter and more clear, I think. > +The optional @var{wp_class} argument defines the type of watchpoint to ^^^^ Since you call it "wp_class", why not use "class" here instead of "type"? You already have @var{type} that has a different role. > +create, if @var{type} is defined as @var{BP_WATCHPOINT}. If a ^^ Two spaces, please (here and elsewhere). > +If a watchpoint type is not provided, it is assumed to be a @var{WP_READ} > +type. ?? Really? WP_WRITE sounds a more logical choice, as it's the default watchpoint type in GDB. > +@defmethod Breakpoint is_valid > +Return @code{True} if this @code{Breakpoint} object is valid, > +@code{False} otherwise. A @code{Breakpoint} object can become invalid > +if the user deletes the breakpoint. In this case, the object still > +exists, but the underlying breakpoint does not. What if execution leaves the scope in which the watchpoint is defined: does this make it invalid? > +This attribute holds the breakpoint's number -- the identifier used by ^^ 3 dashes in a row here, please. > +This attribute holds the breakpoint's type -- the identifier used to Ditto. > +@findex BP_READ_WATCHPOINT > +@findex gdb.BP_READ_WATCHPOINT > +@item BP_READ_WATCHPOINT > +Hardware assisted read watchpoint. > + > +@findex BP_ACCESS_WATCHPOINT > +@findex gdb.BP_ACCESS_WATCHPOINT > +@item BP_ACCESS_WATCHPOINT > +Hardware assisted access watchpoint. > +@end table If we have these BP_* constants, why do we also need the WP_* constants (and the wp_class argument)? > +@defivar Breakpoint location > +This attribute holds the location of the breakpoint, as specified by > +the user. It is a string. If the breakpoint does not have a location > +(that is, it is a watchpoint) an exception will be raised. Is it wise to raise an exception? Why not return something sensible instead? > +the user. It is a string. If the breakpoint does not have an > +expression (the breakpoint is not a watchpoint) an exception will be > +raised. Same here. > +@defivar Breakpoint commands > +This attribute holds the commands attached to the breakpoint. If > +there are commands, this returns a string holding all the commands, ^^^^^^^^^^^^ An attribute cannot "return" anything, it holds a value, right? Thanks.