Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

[Simon Marchi <simark@simark.ca>]

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