Re: [patch] [python] find_line_pc_range

[Phil Muldoon <pmuldoon@redhat.com>]

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 for the patch and your work!

Cheers,

Phil