Re: [patch] [python] Expose some breakpoint operations to Python

[Eli Zaretskii <eliz at gnu dot org>]

> From: Phil Muldoon <pmuldoon@redhat.com>
> Date: Thu, 23 Jun 2011 13:35:02 +0100
> 
> 2011-06-23  pmuldoon  <pmuldoon@redhat.com>
> 
> 	* gdb.texinfo (Breakpoints In Python): Document breakpoint
>           operation APIs.

Thanks.

> +detailed below.  These methods influence how GDB operates with certain
                                                ^^^
"@value{GDBN}"

> +@defop Operation {gdb.Breakpoint} stop (self)
> +The @code{stop} method may be implemented as part of a
> +@code{gdb.Breakpoint}.

I'm not sure we need this sentence.  You already say above that these
methods can be part of sub-classing the gdb.Breakpoint class.  And we
certainly don't need to repeat this sentence for each method.

> +@smallexample
> +class MyBreakpoint (gdb.Breakpoint):
> +
> +   def print_mention (self):
> +     return "Our Breakpoint " + str(self.number) + " at " + str(self.location)
> +@end smallexample

The line that begins with "return" is too long, please break it in two.

> +Python @code{Tuple}.  The first element of the tuple should be a
> +string that defines the breakpoint prelude.  The prelude is printed
> +before the breakpoint location information is printed.  If a prelude
> +is not needed, Python @code{None} can be returned instead.

The last sentence could use some improvement.  First, "can be" is not
really right: using None is a must in this case, AFAICT.  And second,
"returned" is not right either.  How about this variant:

  If no prelude should be printed, the first element should be
  @code{None}.

> +@item PRINT_SRC_AND_LOC
> +Print the source line number and location information associated with
> +this breakpoint.

What is "location" in this context?  This should be clearly
documented, because the example you show is ambiguous.

> +@item PRINT_NOTHING
> +Do not print any breakpoint information.

Would it be a good idea to tell under which circumstances would this
option be useful?

> +The @code{print_one} method may be implemented as part of a
> +@code{gdb.Breakpoint}.  This method will be called when information is
> +requested on a breakpoint (such as the @value {GDBN} command,
> +@samp{info breakpoints}).                                   ^

That comma should be removed.

>                                The @code{address} parameter

Parameters should have the @var markup, not @code.

> +contains the address where @value{GDBN} stopped the inferior.  This
> +method must return a Python @code{Tuple}.  The first element of the
> +tuple should be a Python @code{Long} that contains the address that is
> +to be printed in the informational output, or @code{None}.  If
> +@code{None} is returned, address information will not be printed.  The
> +second element should be a Python string giving a very brief
> +description of the breakpoint, or Python @code{None} if you do not
> +want to print anything for that field.

I think this description should clearly indicate the fields of the
normal output of "info breakpoints" which can be customized by
implementing this method.  Also, what does None mean in terms of
formatting: is the value get replaced by blanks, or do the next fields
get shifted to the left (and thus their formatting gets messed up)?
Or maybe the "Address" column be removed from the display entirely?

The example you show does not answer these questions, because it shows
a more-or-less default display of a breakpoint.

Btw, what's the point of passing the address to this method, if it
should return it?  Is there any reason that users will want to return
some value other than the address itself or None?

> +@defop Operation {gdb.Breakpoint} print_one (self, address)
                                     ^^^^^^^^^
Cut/paste error: this should be print_one_detail.

> +The @code{print_one_detail} method may be implemented as part of a
> +@code{gdb.Breakpoint}.  This method is a companion method to
> +@samp{print_one} (@pxref{print_one}).  This method will be called when
> +information is requested on a breakpoint (such as the @value {GDBN}
> +command, @samp{info breakpoints}), and allows extra details to be
          ^
A redundant comma.

> +printed about that breakpoint.  This method must return a Python
> +string containing brief supplemental information on that breakpoint.

I wonder why we pass the address here.