This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference
On 2018-09-16 10:05, Tom Tromey wrote:
"Simon" == Simon Marchi <simark@simark.ca> writes:
+ if (result == NULL)
+ result = gdbpy_ref<>::new_reference (Py_None);
Simon> I would suggest using Py_RETURN_NONE, which we already use at
many places.
Simon> Instead of setting that result variable, is there something
preventing you
Simon> from returning directly above, like this?
Simon> if (thread_info != NULL)
Simon> return thread_to_thread_object (thread_info).release ();
Simon> The bottom line could just be unconditionally Py_RETURN_NONE.
I made this change.
In the past you couldn't return from inside a try/catch but I think it
is ok now.
I couldn't use Py_RETURN_NONE in py_get_event_thread because it returns
a gdbpy_ref<> -- and I think the latter is more important.
Oh right. While working on this code I though adding a
GDB_Py_REF_RETURN_NONE macro, which would be like Py_RETURN_NONE but
returning a gdbpy_ref. Not sure it's really that useful, since what you
wrote is explicit and clear.
I re-read all of this and I think thread_to_thread_object has a latent
bug. It can return NULL early due to an error:
gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object
(thr->inf));
if (inf_obj == NULL)
return NULL;
or it can return NULL at the end meaning "thread not found".
I am not sure how these two mode of failure differ. Don't we expect the
passed thread_info object to always be valid, and therefore both cases
returning NULL would be an internal GDB logic error?
I think it is best to have a single style - returning NULL should also
set the Python exception.
That makes sense, so the caller can just check for NULL and return NULL
right away (as the snippet above does). We should expect
inferior_to_inferior_object to set the exception on failure.
It seems to me that the second NULL return should just set an
exception. It's not a normal case.
Right.
Responding directly to your next message:
Maybe the gdbpy_ref (1-argument) constructor and release methods could
assert that the Python exception is set if the underlying pointer is
NULL. That would not get full checking but maybe it would catch some
problems. And maybe we should simply use gdbpy_ref in many more places
in the Python layer -- ideally, reserve raw pointers solely for
parameters which are borrowed references.
Makes sense.
Let me know what you think of this.
My apologies again for missing this review and pushing too eagerly.
No problem, LGTM.
Simon