This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |