This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch][python] 5 of 5 - Frame filter documentation changes
- From: Tom Tromey <tromey at redhat dot com>
- To: Phil Muldoon <pmuldoon at redhat dot com>
- Cc: "gdb-patches\ at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 22 Mar 2013 11:14:45 -0600
- Subject: Re: [patch][python] 5 of 5 - Frame filter documentation changes
- References: <513E573C dot 7010502 at redhat dot com>
>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
Phil> This patch/email address documentation changes for Python Frame
Phil> Filters.
Phil> * Python scripting
Phil> +** Frame filters and frame decorators have been added.
Phil> +
Phil> ** Vectors can be created with gdb.Type.vector.
The addition isn't indented properly.
You'll need to tweak this for trunk anyhow, since you'll be starting a
new "Python scripting" entry for 7.7.
Phil> +@node Frame Filter Management
Phil> +@section Management of Frame Filters.
Phil> +@cindex managing frame filters
Phil> +
Phil> +There are several commands available within @value{GDBN} to manage
Phil> +frame filters, detailed here.
I think this should first describe what a frame filter does, in a way
that ordinary users (not Python-writing users) will understand.
Phil> +@kindex set python frame-filter priority
Phil> +@item set python frame-filter priority @var{filter-dictionary} @var{filter-name} @var{priority}
Seeing this in the manual made me wonder why "python" appears here.
I don't think it matters to users. It seems like it is more to type,
without a corresponding benefit.
What do you think of dropping the "python" sub-command?
Phil> +* Frame Filter API:: Filtering Frames.
Phil> +* Frame Decorator API:: Decorating Frames.
Phil> +* Writing a Frame Filter/Decorator:: Writing a Frame Filter and Decorator.
I think this node would be just as clear, and would look better, if it
were just named "Writing a Frame Filter".
Phil> +@node Frame Filter API
Phil> +@subsubsection Filtering Frames.
Phil> +@cindex frame filters api
Phil> +to the @code{priority} attribute in each filter. Once the
Phil> +dictionaries are combined, pruned and sorted, @value{GDBN} then wraps
Phil> +all frames in the call-stack with a @code{FrameDecorator} object, and
Phil> +calls each filter in order. The input to the first frame filter will
I think it should say:
Once the dictionaries are combined, pruned and sorted, @value{GDBN}
creates an iterator which wraps each frame in the call stack in a
@code{FrameDecorator} object, and calls each filter in order.
That is, mention the iterator explicitly. The current text makes it
sound as though the stack is pre-emptively unwound, but that isn't true,
and it is important that developers understand this.
Phil> +@defun FrameDecorator.elided ()
I think these methods should mention 'self'.
That is what we did in the pretty-printing API documentation.
Phil> +The @code{elided} method groups frames together in a hierarchical
Phil> +system. An example would be an interpreter call that occurs over many
Phil> +frames but might be better represented as a group of frames distinct
Phil> +from the other frames.
I think the latter part of this is too vague.
How about -
An example would be an interpreter, where multiple low-level frames make
up a single call in the interpreted language. In this example, the
frame filter would elide the low-level frames and present a single
high-level frame, representing the call in the interpreted language, to
the user.
Phil> +frame decorator. If there are no frames being elided in this frame
Phil> +decorator, this method must return @code{None}.
What happens if it returns an empty iterable?
If that is ok, how about ", this method may return an empty iterable, or
@code{None}"?
Phil> +@defun FrameDecorator.frame_args ()
Phil> +@anchor{frame_args}
Phil> +
Phil> +This method must return an iterable or @code{None}. This iterable
Phil> +must contain objects that implement two methods, described here.
What if it returns an empty iterable instead?
Phil> +The object must implement an @code{argument} method which takes no
Phil> +parameters and must return a @code{gdb.Symbol} (@pxref{Symbols In
Phil> +Python}), or a Python string.
I think it should say "a single @code{self} parameter" rather than "no
parameters". Likewise for the other method.
Phil> +Even if the @code{frame_args} method returns only a single object, it
Phil> +must be iterable.
I would zap this sentence.
Phil> +Even if the @code{frame_locals} method returns only a single object, it
Phil> +must be iterable.
Here too.
Phil> +The second step is registering the frame filter with the dictionary or
Phil> +dictionaries that the frame filter has interest in.
I think this paragraph should xref to "Python Auto-loading" at some
point.
Phil> The global dictionary is always
Phil> +present, and so will the frame filters registered in it.
This reads weirdly.
Phil> +well worth reflecting upon when designing a frame filter. An issue
Phil> +that frame filters should avoid is unwinding the stack if possible.
I think the laziness issue should be pointed out in the main API
documentation somewhere. It's very important; it is fine to repeat it
here, but it should also be somewhere other than in an example.
Phil> +@smallexample
Phil> +class InlinedFrameDecorator(FrameDecorator):
Phil> +
Phil> + def __init__(self, fobj):
Phil> + super(InlinedFrameDecorator, self).__init__(fobj)
Phil> + self.fobj = fobj
self.fobj is never used.
FrameDecorator also stores this in an attribute, "base".
Should that attribute just be declared public?
And if not, shouldn't it be renamed to start with a "_"?
Phil> + def function(self):
Phil> + frame = self.inferior_frame()
Phil> + name = str(frame.name())
Phil> + function = str(frame.function())
Phil> +
Phil> + if frame.type() == gdb.INLINE_FRAME:
Phil> + name = name + " [inlined from "+ function +"]"
This actually violates the FrameDecorator guidelines, since it bypasses
self.base to get the name from the inferior_frame.
It seems like it should call self.base.name() instead.
And, it should have a comment explaining why it needs to use
inferior_frame to call function.
I'm a little surprised that calling the inferior frame's "function" does
what you want here. That seems like a bug TBH.
Phil> +This example approaches the issue with the @code{elided} method. This
Phil> +example is quite long, but very simplistic. It is out-of-scope for
I would say, s/simplistic/simple/.
Phil> + try:
Phil> + eliding_frame = next(self.input_iterator)
Phil> + except StopIteration:
Phil> + return frame
Phil> + return ElidingFrameDecorator(eliding_frame, [frame])
What if there are multiple inline frames in a row?
Wouldn't you want to elide all of them?
That will make the example trickier though.
Tom