Re: [RFA][patch 1/9] Yet another respin of the patch with initial Python support

[Eli Zaretskii <eliz at gnu dot org>]

> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>, drow@false.org,
>         gdb-patches@sourceware.org
> From: Tom Tromey <tromey@redhat.com>
> Date: Sat, 26 Jul 2008 11:04:24 -0600
> 
> >> +python [code]
> >> +  Invoke python code.
> 
> Eli> Please say a few word about the form of [code].  Is it a string? if
> Eli> so, should it be in quotes?
> 
> How about:
> 
>     python [CODE]
>       Invoke CODE by passing it to the Python interpreter.
> 
> I can be as specific as you like here.  I thought, though, that NEWS
> was just a teaser and that users should read the manual for details.

If that's the best we can do, let's at least name the relevant section
of the manual.

> >> +  else if (p1 - p == 6 && !strncmp (p, "python", 6))
> 
> Eli> Can we avoid literal constants such as 6 here, and instead make it
> Eli> dependent on the string "python"?
> 
> Here I just followed the existing code in that file.
> 
> >> else if (p1 - p == 10 && !strncmp (p, "loop_break", 10))
> 
> Eli> Ditto.
> 
> This is existing code.  Please read that file, you'll see there are a
> dozen cases like this.  I don't mind fixing this, but I think it is
> generally preferable to do cleanups in a separate patch.  If you
> disagree, let me know and I will go ahead and mash them together.

I don't mind to fix this as a separate patch.  It's up to you.  I just
want it fixed.

> >> +* Python::                      Scripting @value{GDBN} using Python.
> 
> Eli> I think this should not be a separate chapter, but rather a section of
> Eli> the "Canned Sequences of Commands" chapter.
> 
> This is a drawback of the one-patch-at-a-time submission method.
> Later patches, which we have not yet submitted, greatly expand this
> chapter.  In particular we add a bunch of subsections describing the
> Python API.

As I wrote elsewhere in this thread, I don't see why this should
prevent us from having both CLI and Python scripting in the same
chapter.

> >> +@value{GDBN} exposes a number of features to Python scripts.
> 
> Eli> This should end with a colon, and the list of the features should be a
> Eli> @table.
> 
> I guess those aren't really features, they are more like generic
> things that the python-in-gdb programmer ought to know.
> 
> So, I don't think a table makes sense, but I suppose the current text
> is also misleading.  What do you suggest?

My problem is with the form, not with the content.  By default, each
paragraph in a Texinfo document starts with an indentation, like in a
book.  @table prevents that, producing paragraphs whose all lines line
up.  This section presents a list of features; having each feature's
description begin with an indented line is simply ugly.

You seem to think that @table produces a table, but it doesn't.

> >> +In particular, a @value{GDBN} @code{QUIT} exception is translated to a
> >> +Python @code{KeyboardInterrupt} exception, and other @value{GDBN}
> >> +exceptions are translated to Python @code{RuntimeError}s.
> 
> Eli> I think this is nowhere nearly as clear as it should be.  Even to a C
> Eli> hacker, "QUIT exception" is not sufficiently self-explanatory.  Do you
> Eli> mean SIGQUIT? then please say so.
> 
> Daniel or some other gdb maintainer will have to suggest something.
> I don't know what else to call this.  exceptions.h refers to "QUIT
> event", but also the enum value is "RETURN_QUIT"... maybe the latter?

Now I'm totally confused.  If you are saying that we are now exposing
to user-defined commands some new kind of event, we should at least
document that event.  Just calling it "QUIT exception" doesn't fit
that bill, IMO.

> Eli> Will Python RuntimeError clear enough to casual GDB users who
> Eli> want to use Python scripting, but are not experienced Python
> Eli> programmers?  I'm not sure.
> 
> People expecting to do this will have to read a Python manual.
> There is no way around that.

Let's at least mention the name of the section in the Python manual
where the details can be found.

> Eli> And what about stating explicitly
> Eli> which exceptions map to which Python RuntimeError.
> 
> It does say -- all the rest of them.
> AFAICT gdb really only has quit exceptions and "the other kind".
> 
> Eli> Finally, "GDB exception" is itself a term that is not defined anywhere
> Eli> in the manual.  Strictly speaking, GDB being a C program does not have
> Eli> any exception at all.
> 
> I guess that is true in some pedantic way, if you assume that only
> languages that explicitly have a "throw" statement can have
> exceptions.  But, in practice it is manifestly untrue for gdb.  gdb
> uses exceptions everywhere, and discusses them in just those terms.

We are talking about the user manual.  Readers of the user manual were
up until now ignorant about the internal structure of GDB top-level
code, including how it throws to top level.  If we want users of
Python scripting to take advantage of the mapping between the GDB C
code and Python, we cannot side-step the need to document those GDB
exceptions in some way that is understandable by readers of the user
manual.  Otherwise, only GDB hackers such as you and me will be able
to ever write powerful user-defined commands using Python.

> >> +At startup, @value{GDBN} overrides Python's @code{sys.stdout} and
> >> +@code{sys.stderr} to print using @value{GDBN}'s output-paging streams.
> >> +A Python program which outputs to one of these streams may have its
> >> +output interrupted by the user (@pxref{Screen Size}).  In this
> >> +situation, a Python @code{KeyboardInterrupt} exception is thrown.
> 
> Eli> Is there something here that whoever writes the Python program should
> Eli> know to behave correctly?  Does she need, for example, catch
> Eli> KeyboardInterrupt and do something in the handler?  If so, we should
> Eli> state here what's needed.
> 
> Yes, Python programmers might want to know about this in some
> circumstances.  It just depends on what they are trying to accomplish.
> 
> I don't agree that we need to explain this.  I don't think we should
> try to explain Python programming in general.  Instead, I think we
> should explain the Python API and refer users to already existing
> Python manuals.

The Python manual is a large document.  Saying something about what
needs to be done on the Python side will go a long way towards helping
the reader find the relevant details in the Python manual quickly and
efficiently.  You don't need to describe everything, just hint on it
and use a few relevant keywords.

> >> +@value{GDBN} introduces a new Python module, named @code{gdb}.  All
> >> +methods and classes added by @value{GDBN} are placed in this module.
> 
> Eli> OK, but what does this mean for whoever writes Python extensions for
> Eli> GDB?  While at that, how about explaining why a command FOO is indexed
> Eli> as gdb.FOO?
> 
> I thought it would be nice to index them both ways.  Do you disagree?
> 
> Python programmers know what the module stuff means.  We don't have to
> explain that.

Sorry, I disagree.

> >> +Find the value of a @value{GDBN} setting.
> 
> Eli> "find" or "display"?  If the former, why the name is "show"?
> 
> It mimics the CLI command name.

??? CLI commands that begin with "show" actually display the values.
This one does not, right?

> >> +                                          @var{variable} is a string
> >> +naming the setting to look up; @var{variable} may contain spaces if the
> >> +variable has a multi-part name.  For example, @samp{print object} is a
> >> +valid variable name.
> 
> Eli> So what signals the end of the name?  Also, what exactly is a
> Eli> "multi-part name"?
> 
> The point of this text is to say that you can invoke:
> 
>     gdb.show("print object")
> 
> and get a sensible answer.

Then the text should mention the parens and the quotes.  Specifically,
this:

> >> +@defun show @var{variable}

should have been written like this:

  +@deftypefun show ("@var{variable}")


> >> +Print a string to @value{GDBN}'s paginated standard output stream.
> >> +Ordinarily your program should not call this function directly; simply
> >> +print to @code{sys.stdout} as usual.
> 
> Eli> So why is this command available, and why document it?
> 
> Completeness.  It is visible in the module.

Doesn't sound like a good enough reason to me.