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

[Simon Marchi <simon.marchi@polymtl.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