Re: [patch] [python] find_line_pc_range

[Matt Rice <ratmice at gmail dot com>]

On Sun, Jul 3, 2011 at 9:18 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> Matt Rice <ratmice@gmail.com> writes:
>
>> don't know a whole much about python but,
>> would it be better to return None on error, instead of a tuple containing Nones?
>
> Thanks. I would return either an exception or None depending on the
> situational context of the failure. ?If a None type scenario is expected,
> and normal then it is okay to return that, given that the Python
> scripter would understand the reason for that. ?So documentation is key
> here. ?I think find_line_pc_range is just a utility function,
> which can very well not find the line range. ?So I would replicate that
> functionality in your API, except I would return a single Py_None over a
> tuple.
>
>
>> +salpy_find_line_pc_range (PyObject *self, PyObject *args)
>> +{
>> + ?struct symtab_and_line *sal = NULL;
>> + ?CORE_ADDR start_pc, end_pc;
>> + ?PyObject *start;
>> + ?PyObject *end;
>> + ?PyObject *ret_tuple;
>> +
>> + ?SALPY_REQUIRE_VALID (self, sal);
>> +
>> + ?ret_tuple = PyTuple_New (2);
>
> This call can fail and return NULL, so in this case you need to return
> NULL to propagate the failure.
>
>> + ?if (find_line_pc_range (*sal, &start_pc, &end_pc))
>> + ? ?{
>> +
>> + ? ? ?start = gdb_py_long_from_longest (start_pc);
>> + ? ? ?end = gdb_py_long_from_longest (end_pc);
>> + ? ? ?PyTuple_SET_ITEM (ret_tuple, 0, start);
>> + ? ? ?PyTuple_SET_ITEM (ret_tuple, 1, end);
>> + ? ?}
>
> start and end could be local to this block I think.
>
> Also gdb_py_long_from_longest is macro that points to Py
> Long_FromUnsignedLongLong. ?This can also return NULL which indicates a
> failure, so similar to above, you need to check the return and return
> NULL if one of them fails. ?If one of them does return NULL, you also
> need to appropriately clean-up previous references that were created in
> Python. ?We normally do this by creating a local error: goto.
>
> A minor nit, SET_ITEM does not do any error checking, while SetItem
> does. ?As this is a new tuple, then it is ok to use it. ?But normally
> (and personally, in new tuples) I always use the error checking
> equivalent.
>
>
> + ?else
> + ? ?{
> + ? ? ?PyTuple_SET_ITEM (ret_tuple, 0, Py_None);
> + ? ? ?PyTuple_SET_ITEM (ret_tuple, 1, Py_None);
> + ? ?}
> +
>
> In this case I would just return Py_None (see first paragraph). ?It
> seems a little redundant to return a tuple with Py_None for both
> elements, and this will always be the case on a lookup failure with
> find_line_pc_range. Make sure you incref Py_None before returning it,
> also.
>
> Also, this patch needs tests.

Thanks, attached is an updated patch that also includes tests.

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

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

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

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

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

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

Attachment: foo.diff
Description: Binary data