Re: [PATCH v1 1/4] gdb/python: add gdbpy_borrowed_ref

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

On 17/04/2026 16:40, Tom Tromey wrote:
>>>>>> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> From: Tom Tromey <tom@tromey.com>
>> This adds a new gdbpy_borrowed_ref class.  This class is primarily for
>> code "documentation" purposes -- it makes it clear to the reader that
>> a given reference is borrowed.  However, it also adds a tiny bit of
>> safety, in that conversion to gdbpy_ref<> will acquire a new
>> reference.
> 
> On my branch I ended up giving this a gdbpy_opt_borrowed_ref base class.
> 

Is this branch public ?

>> +/* A class representing a borrowed reference.
>> +
>> +   This is a simple wrapper for a PyObject*.  Aside from documenting
>> +   what the code does, the main advantage of using this is that
>> +   conversion to a gdbpy_ref<> is guaranteed to make a new
>> +   reference.  */
>> +template <class T = PyObject>
>> +class gdbpy_borrowed_ref
> 
> Also mine isn't templated like this though I guess this makes some
> sense.
> 

This templating is handy when you need to access fields from the subclass.

> IIRC I felt like this was overkill.  I didn't read your subsequent
> patches yet but I assume there's some use for it.
> 

For instance, :

static PyObject *
pyuw_create_unwind_info (gdbpy_borrowed_ref<pending_frame_object> pyo_pending_frame,
			 struct frame_id frame_id)
{
   gdb_assert (pyo_pending_frame->frame_info != nullptr);

   gdbpy_heap_type_allocate_or_bailout (unwind_info_wrapper,
				       unwind_info_object,
				       unwind_info_object_type);

   unwind_info_wrapper->frame_id = frame_id;
   unwind_info_wrapper->pending_frame
     = pyo_pending_frame.strong_ref ().release ();
   unwind_info_wrapper->saved_regs = new std::vector<saved_reg>;

   return unwind_info_wrapper.track ().release ();
}

>> +  /* Allow a (checked) conversion to any subclass of PyObject.  */
>> +  template<typename TObj,
>> +	   typename = std::is_convertible<TObj *, T *>>
>> +  operator TObj * ()
>> +  {
>> +    //gdb_assert (PyObject_TypeCheck (m_obj, T::corresponding_object_type));
> 
> No commented-out code.  This is unsafe though.  I would prefer a check.
> 
> I think a name other than "TObj" would be good.
> Even "U" would be better.  "T" and "TObj" are too close.
> 

I deleted this method, it was not needed.

>> +  /* When converting a borrowed reference to a gdbpy_ref<>, a new
>> +     reference is acquired.  */
>> +  template <typename TObj,
>> +	    typename = std::is_convertible<TObj *, T *>>
>> +  operator gdbpy_ref<TObj> ()
>> +  {
>> +    gdb_assert (m_obj != nullptr);
>> +    return gdbpy_ref<TObj>::new_reference (m_obj);
>> +  }
> 
> The naming thing here too.  And also safety if we can get it.
> 

Replaced TObj by U.

   /* When converting a borrowed reference to a gdbpy_ref<>, a new
      reference is acquired.  */
   template <typename U,
	    typename = std::is_convertible<U *, T *>>
   operator gdbpy_ref<U> ()
   {
     if (m_obj != nullptr)
       return gdbpy_ref<U>::new_reference (m_obj);
     return nullptr;
   }

>> +
>> +  gdbpy_ref<T> strong_ref () const noexcept
>> +  {
>> +    return static_cast<gdbpy_ref<T>> (*this);
>> +  }
> 
> I think it would be clearer to directly write this in terms of
> new_reference.

   /* Convert a borrowed reference to a strong one, i.e. gdbpy_ref<>.  */
   gdbpy_ref<T> strong_ref () const noexcept
   {
     if (m_obj != nullptr)
       return gdbpy_ref<T>::new_reference (m_obj);
     return nullptr;
   }

> 
> Tom

Here is the complete version with the modification.
Matthieu

/* A class representing a borrowed reference.

    This is a simple wrapper for a PyObject*.  Aside from documenting
    what the code does, the main advantage of using this is that
    conversion to a gdbpy_ref<> is guaranteed to make a new
    reference.  */
template <class T = PyObject>
class gdbpy_borrowed_ref
{
public:

   gdbpy_borrowed_ref () noexcept
     : m_obj (nullptr)
   {
   }

   gdbpy_borrowed_ref (const std::nullptr_t) noexcept
     : m_obj (nullptr)
   {
   }

   template <typename U,
	    typename = std::is_convertible<U *, T*>>
   gdbpy_borrowed_ref (U *obj) noexcept
     : m_obj (obj)
   {
   }

   template <typename U,
	    typename = std::is_convertible<U *, T*>>
   gdbpy_borrowed_ref (const gdbpy_ref<U> &ref) noexcept
     : m_obj (ref.get ())
   {
   }

   gdbpy_borrowed_ref (const gdbpy_borrowed_ref &other) noexcept
     : m_obj (other.m_obj)
   {
   }

   gdbpy_borrowed_ref &operator= (const gdbpy_borrowed_ref &other)
   {
     m_obj = other.m_obj;
     return *this;
   }

   operator T * () const noexcept
   {
     return m_obj;
   }

   /* Explicit version of the previous implicit cast operator.  */
   T *get () const noexcept
   {
     return m_obj;
   }

   /* When converting a borrowed reference to a gdbpy_ref<>, a new
      reference is acquired.  */
   template <typename U,
	    typename = std::is_convertible<U *, T *>>
   operator gdbpy_ref<U> ()
   {
     if (m_obj != nullptr)
       return gdbpy_ref<U>::new_reference (m_obj);
     return nullptr;
   }

   /* Convert a borrowed reference to a strong one, i.e. gdbpy_ref<>.  */
   gdbpy_ref<T> strong_ref () const noexcept
   {
     if (m_obj != nullptr)
       return gdbpy_ref<T>::new_reference (m_obj);
     return nullptr;
   }

   /* Let users refer to members of the underlying pointer.  */
   T *operator-> () const noexcept
   {
     return m_obj;
   }

private:
   T *m_obj;
};