Re: [PATCH] Add Guile frame filter interface

[Eli Zaretskii <eliz at gnu dot org>]

> From: Andy Wingo <wingo@igalia.com>
> Cc: Andy Wingo <wingo@igalia.com>, Doug Evans <xdje42@gmail.com>
> Date: Sun, 15 Feb 2015 12:27:47 +0100
> 
> The attached patch exposes the frame filter interface to Guile.  It's
> mostly modelled on the Python interface, but has a more functional
> flavor.  No test cases yet, but I figure there will be a round of
> comments so it's probably worth getting this out early.
> 
> Thoughts?

A few comments regarding the documentation part:

First, I think this warrants a NEWS entry.

> +@node Guile Frame Filter API
> +@subsubsection Filtering Frames in Guile
> +@cindex frame filters api

I would add ", guile" to the index entry, to make it more specific.

> +are affected.  The commands that work with frame filters are:
> +
> +@code{backtrace} (@pxref{backtrace-command,, The backtrace command}),
> +@code{-stack-list-frames}
> +(@pxref{-stack-list-frames,, The -stack-list-frames command}),
> +@code{-stack-list-variables} (@pxref{-stack-list-variables,, The
> +-stack-list-variables command}), @code{-stack-list-arguments}
> +@pxref{-stack-list-arguments,, The -stack-list-arguments command}) and
> +@code{-stack-list-locals} (@pxref{-stack-list-locals,, The
> +-stack-list-locals command}).

I don't really like this style of "itemized list with
cross-references", IMO it looks ugly.  Please consider using some more
traditional format, like a @table maybe.  But if you like the results
of this, I won't argue more.

> +reorganize, insert, and remove frames.  @value{GDBN} also provides a
> +more simple @dfn{frame annotator} API that works on individual frames,

In general, any thing you put inside @dfn should have a @cindex entry.

> +There can be multiple frame filters registered with @value{GDBN}, and
> +each one may be individually enabled or disabled at will.  Multiple
> +frame filters can be enabled at the same time.  Frame filters have an
> +associated @dfn{priority} which determines the order in which they are

Here, I question the need for having "priority" in @dfn: it's not new
terminology.

> +should be a number, and which defaults to 20 if not given.  By
> +default, the filter is @dfn{global}, meaning that it is associated

Likewise here for "global".

> +annotated frame is always associated with a GDB frame object.  To
                                               ^^^
@value{GDBN}

> +object that inherits all fields from @var{x}, but whose function name
> +has been set to @code{"foo"}.

I find the results of @code{"foo"} to be awkward, at least in the Info
manual.  I suggest to use @samp{foo} instead.  (I understand that you
wanted to make it clear this is a string, but I think these double
quotes are not really necessary, as everybody understands that it's a
string from what the text says.)

> +@deffn {Scheme Procedure} annotated-frame-arguments ann
> +Return a list of the function arguments associated with the annotated
> +frame @var{ann}.  Each item of the list should either be a GDB symbol
> +(@pxref{Symbols In Guile}), a pair of a GDB symbol and a GDB value
> +(@pxref{Values From Inferior In Guile}, or a pair of a string and a
> +GDB value.  In the first case, the value will be loaded from the frame
> +if needed.

@value{GDBN}, 4 times

> +Annotated frames may also have @dfn{child frames}.  By default, no
> +frame has a child frame, but filters may reorganize the frame stream
> +into a stream of frame trees, by populating the child list.  Of
> +course, such a reorganization is ultimately cosmetic, as it doesn't
> +alter the stack of frames seen by @value{GDBN} and navigable by the
> +user, for example by using the @code{frame} command.  Still, nesting
> +frames may lead to a more understandable presentation of a backtrace.
> +
> +@deffn {Scheme Procedure} annotated-frame-children ann
> +Return a list of the @dfn{child frames} function name associated with

Even the first instance of @dfn{child frames} is questionable, IMO;
having 2 of them is way too much.

> +While frame filters can both reorganize and reannotate the frame
> +stream, it is often the case that one only wants to reannotate the
> +frames in a stream, without reorganizing then.  In that case there is
> +a simpler API for @dfn{frame annotators} that simply maps annotated
> +frames to annotated frames.

You already introduced "frame annotators" earlier, so no need to use
@dfn here.

> +@value{GDBN}.  @var{annotator} should be a function of one argument,
> +takingn annotated frame object and returning a possibily modified
   ^^^^^^^
A typo.

> +@node Writing a Frame Filter in Guile
> +@subsubsection Writing a Frame Filter in Guile
> +@cindex writing a frame filter

This @cindex entry should be qualified with "in guile" or some such.

> +the case, because unlike normal Scheme procedures, @code{stream-cons}
> +is @dfn{lazy} in its arguments, which is to say that its arguments are

Again, @dfn is wrong here.

In all the cases where I commented on incorrect @dfn usage, if what
you wanted was to have the text in italics, just use @emph instead.

Thanks.