Re: [patch] [python] find_line_pc_range

[Matt Rice <ratmice at gmail dot com>]

On Mon, Jul 4, 2011 at 2:13 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> Matt Rice <ratmice@gmail.com> writes:
>
>> Thanks, attached is an updated patch that also includes tests.
>
> Thanks.
>
>> + ? ? ?ret_tuple = PyTuple_New (2);
>> + ? ? ?if (!ret_tuple)
>> + ? ? ? ?return Py_None;
>
> Return NULL here. ?This signifies an error, and that we have abandoned
> the function. ?When a Python function returns NULL, the exception is
> eventually printed and cleared either by the callers, or by the GDB
> Python top-level exception handling code.

Thanks for explaining, that is what I wanted.  You said so in your first email,
but for some reason it didn't latch on, sorry.

>> + ? ? ?start = gdb_py_long_from_ulongest (start_pc);
>> + ? ? ?if (!start)
>> + ? ? goto fail_ret;
>> + ? ? ?if (PyTuple_SetItem (ret_tuple, 0, start) != 0)
>> + ? ? {
>> + ? ? ? Py_XDECREF (start);
>> + ? ? ? goto fail_ret;
>> + ? ? }
>
> You do not need to decrement "start" here. ?Even if the tuple SetItem
> call failed, you have already passed the responsibility to the tuple,
> and you no longer own it (stolen reference). ?SetItem would decrement
> this on failure. ?So just goto to your local error block.

OK, the docs were silent on this regard, and I guessed on error it
wouldn't steal it.

>> + ? ? ?end = gdb_py_long_from_ulongest (end_pc);
>> + ? ? ?if (!end)
>> + ? ? goto fail_ret;
>> + ? ? ?if (PyTuple_SetItem (ret_tuple, 1, end) != 0)
>> + ? ? {
>> + ? ? ? Py_XDECREF (end);
>> + ? ? ? goto fail_ret;
>> + ? ? }
>
> Same as above.
>
>> + ? ? ?fail_ret:
>> + ? ? ? ?Py_XDECREF (ret_tuple);
>
> Small nit, and this is generally ok, but you already know that if this
> error block is called, ret_tuple will be instantiated. So in this case
> the XDECREF is a tiny bit redundant. ?You could just use DECREF here and
> skip the NULL check that XDECREF does. ?Like I said, a tiny nit, but I
> just thought I would point it out.

k.

>> + ? ? return Py_None;
>
> If there is a Python exception that you cannot handle internally to your
> function, you must always return NULL. ?This informs callers, and
> ultimately the exception managing code in GDB that some Python call
> failed, to print the exception, and clear it.
>
>> @@ -526,6 +573,8 @@ static PyMethodDef sal_object_methods[] = {
>> ? ?{ "is_valid", salpy_is_valid, METH_NOARGS,
>> ? ? ?"is_valid () -> Boolean.\n\
>> ?Return true if this symbol table and line is valid, false if not." },
>> + ?{ "find_line_pc_range", salpy_find_line_pc_range, METH_NOARGS,
>> + ? ?"Return a tuple of start and end ranges for the symbol table." },
>> ? ?{NULL} ?/* Sentinel */
>> ?};
>
> We try to put the return type in the help now as a formal statement.
> Look at the two-line help example above: "is_valid()" for guidance.

Updated that text a little bit also, along with using Eli's text for the manual.

2011-07-04  Matt Rice  <ratmice@gmail.com>

        * python/py-symtab.c: Populate sal_object_methods.
        (salpy_find_line_pc_range): New function.

 2011-07-04  Matt Rice  <ratmice@gmail.com>
                   Eli Zaretskii <eliz@gnu.org>

        * gdb.texinfo (Symbol Tables In Python): Add find_line_pc_range method.

2011-07-04  Matt Rice  <ratmice@gmail.com>

        * gdb.python/py-symtab.exp: New Tests for find_line_pc_range.

Attachment: foo.diff
Description: Binary data