Re: [PATCH v2 5/9] gdb/python: allow ref_ptr<T, Policy>::new_reference to accept subclasses of T

[Matthieu Longo <matthieu.longo@arm.com>]

On 06/03/2026 16:47, Tom Tromey wrote:
>>>>>> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> After a deeper examination, I am not sure that your series helps much for this patch.
> 
>> * Case 1: return value from the registry.
> 
>> PyObject *result = (PyObject *) cfpy_inferior_corefile_data_key.get (inf);
>>    if (result != nullptr)
>>      return gdbpy_ref<>::new_reference (result);
> 
>> If I had gdbpy_borrowed_ref available, I could replace the code above by something like:
>> gdbpy_borrowed_ref result = cfpy_inferior_corefile_data_key.get (inf);
>> if (result != nullptr)
>>    return result; /* assuming that gdbpy_ref<> would have a constructor taking gdbpy_borrowed_ref. */
> 
> Ok, I see.  Sorry about that.
> 
> Does the registry need to return a properly-typed one?

In my understanding, it does not.
I guess you would like to change

static const registry<inferior>::key<corefile_object,
				     inferior_corefile_deleter>
      cfpy_inferior_corefile_data_key;

to

static const registry<inferior>::key<PyObject,
				     inferior_corefile_deleter>
      cfpy_inferior_corefile_data_key;

But the only impact of this is removing the cast to (PyObject *).
Today we can already write:
   gdbpy_ref<> result (cfpy_inferior_corefile_data_key.get (inf));

I personally don't like this syntax, and would prefer something like:

   gdbpy_borrowed<> key<gdbpy_ref<>, inferior_corefile_deleter>::get (inferior *) const;

And then in this function:

   gdbpy_borrowed<> corefile_obj = cfpy_inferior_corefile_data_key.get (inf);
   if (corefile_obj != nullptr)
     return corefile_obj;

It seems more natural to me.

> Perhaps it can just always return a gdbpy_ref<>.
> 

I am not sure how to do this with the current implementation of registry.

Why not using something like an std::unordered_map<inferior *, gdbpy_ref<corefile_object>> ?
The only impediment seems to be this clean-up routine setting corefile_object->inferior to nullptr.
I am not really understanding why this is required. Could this setting to null be removed completely ?

>> * Case 2: registration of the new object in the registry and return the value.
> 
>> gdbpy_ref<corefile_object> object
>>      (PyObject_New (corefile_object, &corefile_object_type));
>> ...
>> cfpy_inferior_corefile_data_key.set (inf, object.get ());
>> return gdbpy_ref<>::new_reference (object.release ());
> 
> Here the release stuff is no longer needed as upcasting works fine:
> 

The release() is not required for upcasting, but keeping the reference counter
incremented to count for the instance stored inside the registry.

/* Allocation of object via PyObject_New() */
gdbpy_ref<corefile_object> object
     (PyObject_New (corefile_object, &corefile_object_type));
...

/* Save the object in the registry which only holds a raw pointer.
    If this registry was storing gdbpy_ref<> and set() was supporting
    both copy and move semantics, using set(const &gdbpy_ref<>) would
    simplify the code on the return to a simple "return value;".  */
cfpy_inferior_corefile_data_key.set (inf, object.get ());

/* Return a new reference. The copy is not sufficient because the gdbpy_ref<>
    would go out of scope at the end of the function, and the registry would
    potentially hold a dangling pointer as soon as the return value would go
    out of scope somewhere in the calling context. */
return gdbpy_ref<>::new_reference (object.release ());

> cfpy_inferior_corefile_data_key.set (...);
> return object;
> 
>> However, I am getting further from my original objective, which was only flattening the code for upcoming changes.
>> All those improvements seem to me out of scope of my original patch.
> 
> Yeah.  It's fine to take small steps and not do too much.

If we decide to change the implementation of the registry later to store gdbpy_ref<>, the code should simplify itself.
However, as explain before, the replacement of a raw pointer by gdbpy_ref<> does not look trivial.
Consequently, I propose to keep the code as it is for now.

> So if it's convenient just go ahead; but if you don't mind add a bit of
> explanation to the commit message.
> 

If I stick to the original goal of this patch, i.e. allowing ref_ptr<T, Policy>::new_reference to accept subclasses of T, it seems to me that the description is clear enough.
What would you like to add ?

> thanks,
> Tom

Matthieu