Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref

[Pedro Alves <palves@redhat.com>]

On 01/23/2017 10:40 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> The functions inferior_to_inferior_object and find_inferior_object
> return a new reference to an inferior_object.  This means that the
> caller owns that reference and is responsible for decrementing it when
> it's done.  To avoid the possibility of the caller forgetting to DECREF
> when it's done with the reference, make those functions return a
> gdbpy_inf_ref instead of a plain pointer.

I like this style of API.  I've argued for it before too.

> If the caller doesn't need the reference after it has used it,
> gdbpy_inf_ref will take care of removing that reference.  If the
> reference needs to outlive the gdbpy_inf_ref object (e.g. because we are
> return the value to Python, which will take ownership of the reference),
> the caller will have to release the pointer.  At least it will be
> explicit and it won't be ambiguous.
> 
> I added comments in inferior_to_inferior_object for the poor souls who
> will have to deal with this again in the future.
> 
> A couple of things I am not sure about:
> 
>   * I am not sure whether the behaviour is right with the assignment
>   operator in delete_thread_object, so if somebody could take a look at
>   that in particular it would be appreciated:
> 
>     gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
> 
>   I suppose it's the operator= version which moves the reference that is
>   invoked?

Since this is initialization, op= is not called.  This either
calls the copy constructor, or find_inferior_object constructs the
object that it returns directly on top of &inf_obj_ref
(i.e., no copy at all) [RVO/NRVO].

http://stackoverflow.com/questions/2847787/constructor-or-assignment-operator
https://en.wikipedia.org/wiki/Return_value_optimization

> @@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile)
>     representing INFERIOR.  If the object has already been created,
>     return it and increment the reference count,  otherwise, create it.
>     Return NULL on failure.  */
> -inferior_object *
> +gdbpy_inf_ref
>  inferior_to_inferior_object (struct inferior *inferior)
>  {
...
> -      if (!inf_obj)
> -	  return NULL;
> +      if (inf_obj == NULL)
> +	return gdbpy_inf_ref ();

You shouldn't need changes like this one.  gdbpy_ref has an
implicit ctor that takes nullptr_t exactly to allow implicit
construction from null.
>  

> @@ -304,39 +303,34 @@ add_thread_object (struct thread_info *tp)
>  static void
>  delete_thread_object (struct thread_info *tp, int ignore)
>  {
> -  inferior_object *inf_obj;
>    struct threadlist_entry **entry, *tmp;
>  
>    if (!gdb_python_initialized)
>      return;
>  
>    gdbpy_enter enter_py (python_gdbarch, python_language);
> +  gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
>  
> -  inf_obj
> -    = (inferior_object *) find_inferior_object (ptid_get_pid (tp->ptid));
> -  if (!inf_obj)
> +  if (inf_obj_ref == NULL)
>      return;
>  
>    /* Find thread entry in its inferior's thread_list.  */
> -  for (entry = &inf_obj->threads; *entry != NULL; entry =
> -	 &(*entry)->next)
> +  for (entry = &inf_obj_ref.get ()->threads;

Hmm, changes like these are odd.  gdbpy_ref has an operator->
implementation, so inf_obj->threads should do the right thing?

> @@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void *datum)
>  PyObject *
>  gdbpy_selected_inferior (PyObject *self, PyObject *args)
>  {
> -  return (PyObject *) inferior_to_inferior_object (current_inferior ());
> +  gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));

If the function returns gdbpy_inf_ref already, I much prefer
using = initialization over (), like:

  gdbpy_inf_ref inf_obj_ref
     = inferior_to_inferior_object (current_inferior ());

The reason is that this makes it more obvious what is going on.
The ctor taking a PyObject* is explicit so inferior_to_inferior_object
must be returning a gdbpy_inf_ref.

With:

  gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));

one has to wonder what constructor is being called, and whether there's
some kind of explicit conversion going on.

So the = version is more to the point and thus makes it
for a clearer read because there's less to reason about.

> +
> +  /* Release the reference, it will now be managed by Python.  */
> +  return (PyObject *) inf_obj_ref.release ();
>  }

Thanks,
Pedro Alves