Re: [patch][python] Add breakpoint support.

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Mon, 29 Mar 2010 14:51:31 +0100
> From: Phil Muldoon <pmuldoon@redhat.com>
> 
> 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.