This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] [python] find_line_pc_range
- From: Phil Muldoon <pmuldoon at redhat dot com>
- To: Matt Rice <ratmice at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 03 Jul 2011 17:18:21 +0100
- Subject: Re: [patch] [python] find_line_pc_range
- References: <CACTLOFptUbx6vtmykDBxtqv3GcrGTQXzX4m6q6O4X4ddw2WM1w@mail.gmail.com>
- Reply-to: pmuldoon at redhat dot 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