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

Tom Tromey
2008-07-26T17:04:58Z

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Do I understand correctly that I need not review the doco patches you
Eli> posted prior to this? If there's something else I should review,
Eli> please point to the corresponding messages.

This is it.

>> + GDB now has support for scripting using Python. Whether this is
>> + available is determined at configure time; you can explicitly
>> + disable the support using --without-python.

Eli> Is --without-python a configure-time switch or a run-time switch? If
Eli> the former, I don't think it should be mentioned here. If the latter,
Eli> we should explicitly say it's a run-time switch.

Configure time. I changed the text to read:

* Python scripting

GDB now has support for scripting using Python. Whether this is
available is determined at configure time.

>> +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.

>> + 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.

>> +* 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.

>> +@node Python
>> +@chapter Scripting @value{GDBN} using Python
>> +@cindex Python

Eli> First, @cindex entries should begin with lower-case letters, to
Eli> produce deterministic results when index entries are sorted.

Ok. I fixed this. You might want to fix the other 121 occurrences ;)

>> + This feature is available only if
>> +@value{GDBN} was configured using @code{--with-python}.

Eli> Please use @option for switches, not @code.

Note there's another use of @code in this situation. I keep copying
the wrong examples :-(

>> +@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?

>> +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?

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.

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.

>> +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.

>> +@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.

>> +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.
"find" would be weird, IMO. I'm open to other names.

>> + @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.
If you want to rephrase this somehow, that is fine by me.

>> +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.

Tom