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

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Fri, 30 Nov 2012 14:31:59 +0000
> From: Phil Muldoon <pmuldoon@redhat.com>
> 
> This patch/email address documentation changes for Python Frame
> Filters.

Thanks.

What about NEWS?

> +@item backtrace raw
> +@itemx bt raw
> +@itemx bt raw @var{n}
> +@itemx bt raw -@var{n}
> +@itemx bt raw full
> +@itemx bt raw full @var{n}
> +@itemx bt raw full -@var{n}
> +Do not run Python frame filters on this backtrace.  @xref{Frame
> +Filters API}, for more information.  This is only relevant when
> +@value{GDBN} has been configured with @code{Python} support.

I'd prefer 'no-filters' or some such, since 'raw' has no immediate
relation to "frame filters".

I suggest to mention here that frame filters can be disabled, with a
cross-reference to where commands to disable them are described.
Also, I think we should have a CLI commands to enable and disable them
all, because otherwise I see no simple way of getting
backward-compatible behavior.

> +@node Frame Filters API
> +@subsubsection Filtering and Wrapping Frames.
> +@cindex Frame Filter/Wrappers API

There's a separate node about frame wrapper API, so I think this
@cindex entry should be modified accordingly.  Also, please start
index entries with a lower-case letter, unless caps are absolutely
necessary (as in proper names).

> +@value{GDBN}.  Frame filters may only work on the wrapping iterator.
> +This preserves data integrity within @value{GDBN}.

Not sure the reader will understand what you mean by "wrapping
iterator" here.  Perhaps explain or give a cross-reference.

> +takes a Python iterator, and returns a Python iterator.  For further
> +information on frame filters see, @ref{Writing a Frame
> +Filter/Wrapper}.                ^

No need for this comma.

> +@node Frame Wrapper API
> +@subsubsection Wrapping and Decorating Frames.
> +@cindex Frame Wrapper API

Index entries should start with a lower-case letter.

> +@defun FrameWrapper.elided ()
> +
> +The @code{elided} method groups frames together in a hierarchical
> +system.  An example would be an interpreter call that occurs over many
> +frames but might be better represented as a group of frames distinct
> +from the other frames.
> +
> +The @code{elide} function must return an iterator that conforms to the
> +Python iterator protocol.

"elide" or "elided"?

>                             This iterator must contains the frames that
                                            ^^^^^^^^^^^^^
Either "must contain" or "contains" without "must".

> +It is the frame filter task to also filter out the elided frames from
                   ^^^^^^
"filter's"

> +the source iterator.  This will avoid the frame being printed twice.
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"This will avoid printing the frame twice."

> +@defun FrameWrapper.filename ()
> +
> +This method returns the filename associated with this frame.
> +
> +This method must return a Python string containing the filename, and
> +optionally, the path to the filename of the frame, or a Python
> +@code{None}.

So does this method return one string or two, when it uses the
optional behavior?  IOW, it is not clear whether "and" means another
output or the contents of a single output string.

> +@defun FrameWrapper.frame_args ()
> +
> +This method must return an  iterator that conforms to the Python
> +iterator protocol, or a Python @code{None}.  This iterator must
> +contain objects that implement two methods, described here.
> +
> +The object must implement an @code{argument} method which takes no
> +parameters and must return a @code{gdb.Symbol} or a Python string.  It
> +must also implement a @code{value} method which takes no parameters
> +and which must return a @code{gdb.Value}, a Python value, or
> +@code{None}.  If the @code{value} method returns a Python @code{None},
> +and the @code{argument} method returns a @code{gdb.Symbol},
> +@value{GDBN} will look-up and print the value of the @code{gdb.Symbol}
> +automatically.

I would suggest to describe what are the symbol and the value here.
Maybe the reader will be able to guess that, but why expect them to
guess?

> +A brief example:
> +
> +@smallexample
> +class SymValueWrapper ():
> +
> +    def __init__(self, symbol, value):
> +        self.sym = symbol
> +        self.val = value
> +
> +    def value (self):
> +        return self.val
> +
> +    def symbol (self):
> +
> +        return self.sym

Is this example (which AFAIU simply shows what the default of None
will do) really useful?  A useful example should show some non-trivial
use of the facility.

> +@defun FrameWrapper.frame_locals ()
> +
> +This method must return an iterator that conforms to the Python
> +iterator protocol, or a Python @code{None}.  This iterator must
> +contain objects that implement two methods, described here.
> +
> +The object must implement an @code{argument} method which takes no
> +parameters and must return a @code{gdb.Symbol} or a Python string.  It
> +must also implement a @code{value} method which takes no parameters
> +and which must return a @code{gdb.Value}, a Python value, or
> +@code{None}.  If the @code{value} method returns a Python @code{None},
> +and the @code{argument} method returns a @code{gdb.Symbol},
> +@value{GDBN} will look-up and print the value of the @code{gdb.Symbol}
> +automatically.

Likewise here: a description of what the symbol and the value express
would be beneficial.

> +@node Writing a Frame Filter/Wrapper
> +@subsubsection Writing a Frame Filter and Wrapper
> +@cindex Writing a Frame Filter/Wrapper

Lower-case letters in index entries, please.

> +The Python dictionary @code{gdb.frame_filters} contains key/object
> +pairings that compromise a frame filter.  These frame filters must
                 ^^^^^^^^^^
I assume you meant "comprise".

> +Auto-loading}).  The two other areas where frame filter dictionaries
> +can be found are: @code{gdb.Progspace} which contains a
> +@code{frame_filters} dictionary attribute, and each @code{gdb.Objfile}
> +object which also contain a @code{frame_filters} dictionary attribute.
                     ^^^^^^^
"contains"

> +
> +Each frame filter object in these dictionaries is passed a single
> +Python iterator argument and should return a Python iterator.  Each
> +frame filter object must conform to the frame filter interface
> +definition (@pxref{Frame Filters API}).  The iterator returned by the
> +frame filter must contain only a collection of frame wrappers
> +(@pxref{Frame Wrapper API}), conforming to the frame wrapper interface
> +definition.
> +
> +When a command is executed from @value{GDBN} that is compatible with
> +frame filters, @value{GDBN} combines the @code{global},
> +@code{gdb.Progspace} and all @code{gdb.ObjFile} dictionaries
> +currently loaded.  All of the @code{gdb.Objfile} dictionaries are
> +combined as several frames and thus object files might be in use.
           ^                 ^                     ^
I think you need commas in the indicated places, or else this sentence
can be misinterpreted.  Also I'd insert "several" before "object", for
the same reason.  That's assuming I understood correctly what you
meant in that sentence.

> +@value{GDBN} then prunes any frame filter where the @code{enabled}
> +attribute is set to @code{False}.

"any frame filters whose @code{enabled} attribute is @code{False}."

> +according to the @code{priority} attribute in each filter.  Once the
> +dictionaries are combined, sorted and pruned, @value{GDBN} then wraps
                              ^^^^^^^^^^^^^^^^^
"pruned and sorted" (as this is the order you just described).

> +all frames in the call-stack with a @code{BaseFrameWrapper} object,
> +and calls each filter in priority order.

The "priority" part should not be here, since the filters are already
sorted, right?

>                                         The input to the first frame
> +filter will be an initial iterator wrapping a collection of
> +@code{BaseFrameWrapper} objects.  The output from the previous filter
> +will always be the input to the next filter, and so on.

I would suggest to move all the text of this node up to here to the
"Frame Filters API" section.  This text provides an overview of how
filters and wrappers work, which IMO is sorely missed before you dive
into describing the API itself.  Having this overview there will allow
the reader to better understand what you describe in the correct
context.  In any case, the text up to here has almost nothing to do
with "writing a filter/wrapper", which is the subject of this section.

> +@subsubheading Implementing a frame filter

If you take my suggestion above, this subsubheading will become
unnecessary.

> +The first step is attribute creation and assignment:
> +
> +@smallexample
> +        self.name = "Foo"
> +        self.priority = 100
> +        self.enabled = True
> +@end smallexample
> +
> +The second step is registering the frame filter with the dictionary or
> +dictionaries that the frame filter has interest in:
> +
> +@smallexample
> +        gdb.frame_filters [self.name] = self
> +@end smallexample

Instead of repeating the code fragments, how about adding comments to
the example that explain what its various parts do?

> +@item disable frame-filter @var{filter-dictionary} @var{filter-name}
> +Disable a frame filter in the dictionary matching
> +@var{filter-dictionary} and @var{filter-name}.
> +@var{filter-dictionary} may be @code{global}, @code{progspace} or the
> +name of the object file where the frame filter dictionary resides.
> +@var{filter-name} is the name of the frame filter.  A disabled
> +frame-filter is not deleted, it may be enabled again later.

It would be good to say here how to disable all filters at once.  (I
hope there is such a way.)

> +(gdb) info frame-filter
> +
> +global frame-filters:
> +  Priority  Enabled  Name
> +  ========  =======  ====
> +  1000      No       Capital Primary Function Filter
> +  100       Yes      Reverse
> +progspace /build/test frame-filters:
> +  Priority  Enabled  Name
> +  ========  =======  ====
> +  100       Yes      Progspace Filter 1
> +objfile /build/test frame-filters:
> +  Priority  Enabled  Name
> +  ========  =======  ====
> +  999       Yes      Build Test Program Filter

Wouldn't it be better to display all the filters in a single table,
adding the dictionary as another column?

> +(gdb) disable frame-filter ``/build/test'' ``Build Test Program Filter''

Why the quotes? they were never mentioned before.  And in any case,
use ".." here, not ``..'', as the conversion is not done in @example,
AFAIR.

> +@subheading The @code{-enable-frame-filters} Command
> +@findex -enable-frame-filters
> +
> +@smallexample
> +-enable-frame-filters
> +@end smallexample
> +
> +@value{GDBN} allows Python-based frame filters to affect the output of
> +the MI commands relating to stack traces.  As there is no way to
> +implement this in a fully backward-compatible way, a front end must
> +request that this functionality be enabled.
> +
> +Once enabled, this feature cannot be disabled.

So in CLI, each filter can be enabled and disabled individually, but
in MI they can only be enabled en masse, and cannot be disabled?  is
that a good idea?

> +This feature is currently (as of @value{GDBN} 7.6) experimental, and
> +may work differently in future versions of @value{GDBN}.

Do we have to have this version-specific note in the manual?  They are
maintenance burden in the long run.