Re: [patch][python] 5 of 5 - Frame filter documentation changes

[Tom Tromey <tromey@redhat.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