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

[Phil Muldoon <pmuldoon at redhat dot com>]

On 12/07/2012 03:04 PM, Eli Zaretskii wrote:
>> Date: Fri, 07 Dec 2012 14:34:50 +0000
>> From: Phil Muldoon <pmuldoon@redhat.com>
>> CC: gdb-patches@sourceware.org
>>
>>>> +@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"?
>>
>> I don't care, but this function returns frames that have been elided
>> so it made sense to me.  The eliding itself is done in the frame
>> filter or in another Python object.
> 
> No, I meant that to say that the method is called "elided", but the
> last sentence uses "elide".  Does that reference some other function?


No it sure doesn't.  Typo.  I will fix it in the next patch.
 
>>>> +@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?
>>
>> I really struggled with this section.  As value and symbol are not
>> simple things to describe in the scope of this function definition.
> 
> Isn't the symbol the name of an argument and the value its
> corresponding value?  If so, just say that.

Ok, thanks will do.

>>>>                                         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.
>>
>>
>> What I tried to do with this functionality is provide four sections
>> that stood independently.  Frame Filters API and Frame Wrappers API
>> which were API references.  These would form a library of the API
>> calls.  But writing a frame filter also implies knowledge of frame
>> wrappers.  And putting this in the Frame Filters API seemed the wrong
>> place.  It's an API section, not a description of how frame filters
>> and frame wrappers work together. So my opinion was that API sections
>> should only reference API like content.  So I decided to write a
>> cookbook like section. This section, with suitable cross-references,
>> brought the knowledge stored in the two API sections and approached
>> the problem of how to write a Frame Filter and Frame Wrapper with an
>> example focused approach.  I would still like to maintain this
>> approach if I can.  (The last section I have not talked about here is
>> Managing Frame Filters, with GDB commands).
> 
> You are IMO assigning too much importance to the names of the
> sections.  Even if your concerns are justified, renaming the sections
> is easy.

Ah I think I misread your initial request. I did not parse the "up to
here" phrase in conjunction with the position of your in-line comments
with the patch; I thought you wanted to move the whole node into frame
filters node.  Thanks for explaining.

>>>> +(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.
>>
>> Quotes are needed so that options with spaces are correctly parsed
>> (like the name).  Quotes are not needed for the first "/build/test"
>> option.  I will remove those.  EMACS is quite insistent on placing
>> those quotes in @smallexample.
> 
> You are right about Emacs, but that's an Emacs bug.  It doesn't do
> that inside @example.
> 
>> In the PDF output I think they are output correctly, but I will fix.
> 
> They certainly don't look right in the Info output.

Ok, will fix.
 
>>>> +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.
>>
>> I added this as the pretty printers feature also has a similar
>> statement.  I can remove it.
> 
> Please do remove it, version-specific information in the manual is a
> maintenance burden.

Will do, thanks

Cheers

Phil