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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Thanks for the comments.  I'll update my branch, but I'll wait until 
Tom's series is pushed and see what's still relevant in mine.

On 2017-02-09 07:30, Pedro Alves wrote:
> > @@ -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.

Ok.  This required adding the corresponding constructor in 
gdbpy_ref_base:

    gdbpy_ref_base (const std::nullptr_t)
    : gdb::ref_ptr<T, gdbpy_ref_policy<T>> (nullptr)
    {
    }

> >    /* 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?

Hmm you're right, not sure why I added those.

> > @@ -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.

Right, it's more obvious.

Thanks,

Simon