[PATCH 5/5] gdb/python: document GDB/MI commands in Python

[Jan Vrany]

Hi, 
> 
> > Date: Mon, 17 Jan 2022 12:44:25 +0000
> > From: Jan Vrany via Gdb-patches <gdb-patches@sourceware.org>
> > Cc: Jan Vrany <jan.vrany@labware.com>
> > 
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -146,6 +146,8 @@ show debug lin-lwp
> >    ** New function gdb.host_charset(), returns a string, which is the
> >       name of the current host charset.
> >  
> > 
> > 
> > 
> > +  ** New GDB/MI commands can now be written in Python.
> 
> The "new" part here took me by surprise.  Why do we need it?
> 
> Or how about this rewording:
> 
>   ** It is now possible to add GDB/MI commands implemented in Python.
> 
> > +* GDB/MI Commands In Python::   Implementing new @sc{GDB/MI} commands in Python.
>                                                     ^^^^^^^^^^^
> The argument of @sc should be in lower case.
> 
> > +@cindex CLI commands in python
> >  @cindex commands in python
> >  @cindex python commands
> >  You can implement new @value{GDBN} CLI commands in Python.  A CLI
> > @@ -4092,6 +4095,71 @@ registration of the command with @value{GDBN}.  Depending on how the
> >  Python code is read into @value{GDBN}, you may need to import the
> >  @code{gdb} module explicitly.
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > +@node GDB/MI Commands In Python
> > +@subsubsection @sc{GDB/MI} Commands In Python
> > +
> > +@cindex MI commands in python
> > +@cindex commands in python
> > +@cindex python commands
> 
> You've added a second copy of the last two index entries, without any
> qualifier.  This will cause makeinfo to generate index entries
> "commands in python" and "commands is python<2>", without giving the
> reader any clue what is the difference between these two instances.
> 
> It is much better to qualify each instance with some unique
> qualifier.  Here, I'd use ", CLI" and ", GDB/MI" as the qualifiers.

I see. However I'm not sure what do you mean by "qualifiers". 
In new version (which I'll submit  once I get review on
other patches in this series) I changed index entries to 

@node CLI Commands In Python
@subsubsection CLI Commands In Python

@cindex commands in python @subentry CLI
@cindex python commands @subentry CLI

and then 

@node GDB/MI Commands In Python
@subsubsection @sc{gdb/mi} Commands In Python

@cindex commands in python @subentry @sc{gdb/mi}
@cindex python commands @subentry @sc{gdb/mi}

It looks good to me in HTML version. 

Is that what you meant? It seems that this is the first use
of @subentry in @cindex in GDB manual so perhaps you meant
something different? 


> 
> > +You can implement new @sc{GDB/MI} (@pxref{GDB/MI}) commands in Python.
> 
> Same comment as for the NEWS entry.  With a possibly similar solution.

This phrasing with "new" (both here and in NEWS) was taken from entries
for CLI commands. For NEWS, I have changed it as you suggested, it's better.

But for manual it makes less sense to me. Wording "It is now possible to add GDB/MI 
commands implemented in Python." sounds to me as to suggest this is some
new feature. It makes sense for NEWS but perhaps might look bit weird after
5 years (given that this series gets in) when this is no new feature. 

Maybe I'm misunderstanding something...

> 
> > +@var{name} is the name of the command.  It must be a valid @sc{GDB/MI}
> > +command and must start with hyphen (-).
> 
> What does "It must be a valid @sc{GDB/MI} command" mean here?  Did you
> mean to say "It must be a valid @sc{GDB/MI} command name" instead?  If
> so, I suggest
> 
>   It must be a valid name of a @sc{GDB/MI} command, and in particular
>   must start with a hyphen (@samp{-}).
> 
> > +@var{arguments} is a list of strings.  Note, that @code{--thread}
> > +and @code{--frame} arguments are handled by @value{GDBN} itself therefore
> > +they do not show up in @code{arguments}.                       ^
> 
> Comma missing there.
> 
> > +If this method throws an exception, it is turned into a @sc{GDB/MI}
> 
> "it" here is ambiguous: does it mean the exception or does it mean the
> method?  Suggest to be more explicit:
> 
>   If this method throws an exception, the exception is turned into a
>   @sc{GDB/MI} @code{^error} response.
> 
> > +@itemize
> > +@item If the value is Python sequence or iterator, it is converted to
> > +@sc{GDB/MI} @emph{list} with elements converted recursively.
> 
> In an @itemize list, eacg @item should be alone on its line (unlike in
> a @table).
> 
> > +@item If the value is Python dictionary, it is converted to
> > +@sc{GDB/MI} @emph{tuple}.
> 
> I don't think using @emph here is a good idea.  I think @code is
> better.  @mph will look odd in Info.
> 
> 

I changed all the rest as suggested.

Thanks! Jan