public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Jan Vrany <jan.vrany@labware.com>
To: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [PATCH 5/5] gdb/python: document GDB/MI commands in Python
Date: Tue, 18 Jan 2022 12:34:00 +0000	[thread overview]
Message-ID: <41f128101c14a2794decf8a25b5a3940234d68e6.camel@labware.com> (raw)
In-Reply-To: <83k0eywlar.fsf@gnu.org>

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


  reply	other threads:[~2022-01-18 12:34 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-17 12:44 [PATCH 0/5] create GDB/MI commands using python Jan Vrany
2022-01-17 12:44 ` [PATCH 1/5] gdb/mi: introduce new class mi_command_builtin Jan Vrany
2022-01-17 12:44 ` [PATCH 2/5] gdb/python: create GDB/MI commands using python Jan Vrany
2022-02-06 16:52   ` Lancelot SIX
2022-01-17 12:44 ` [PATCH 3/5] gdb/python: allow redefinition of python GDB/MI commands Jan Vrany
2022-02-06 17:13   ` Lancelot SIX
2022-02-06 20:33     ` Simon Marchi
2022-02-06 20:44       ` Jan Vrany
2022-02-06 20:46         ` Simon Marchi
2022-02-07  9:46         ` Lancelot SIX
2022-01-17 12:44 ` [PATCH 4/5] gdb/testsuite: add tests for python-defined MI commands Jan Vrany
2022-01-17 12:44 ` [PATCH 5/5] gdb/python: document GDB/MI commands in Python Jan Vrany
2022-01-17 13:15   ` Eli Zaretskii
2022-01-17 13:20   ` Eli Zaretskii
2022-01-18 12:34     ` Jan Vrany [this message]
2022-01-18 15:09       ` Eli Zaretskii
2022-01-18 13:55 ` [PATCH 0/5] create GDB/MI commands using python Andrew Burgess
2022-01-18 15:13   ` Jan Vrany
2022-01-21 15:22     ` Andrew Burgess
2022-01-24 12:59       ` Jan Vrany
2022-02-02 16:57         ` Andrew Burgess
2022-02-06 21:16       ` Simon Marchi
2022-02-07 15:56         ` [PATCHv2] gdb/python/mi: create MI " Andrew Burgess
2022-02-08 15:16           ` Simon Marchi
2022-02-09 12:25             ` [PATCHv3] " Andrew Burgess
2022-02-09 14:08               ` Simon Marchi
2022-02-10 18:26                 ` Andrew Burgess
2022-02-13 14:27                   ` Joel Brobecker
2022-02-13 21:46                     ` Jan Vrany
2022-02-24 10:37               ` [PATCHv4] " Andrew Burgess
2022-02-25 19:22                 ` Tom Tromey
2022-02-25 19:31                   ` Jan Vrany
2022-02-28 16:48                 ` [PATCHv5] " Andrew Burgess
2022-02-28 18:40                   ` Tom Tromey
2022-03-13  4:47                   ` Joel Brobecker
2022-03-14 14:13                     ` Andrew Burgess
2022-03-16  8:10                       ` Joel Brobecker
2022-03-16 12:29                       ` Simon Marchi
2022-03-18 15:06                   ` Simon Marchi
2022-03-18 16:12                     ` Andrew Burgess
2022-03-18 19:57                       ` Simon Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=41f128101c14a2794decf8a25b5a3940234d68e6.camel@labware.com \
    --to=jan.vrany@labware.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).