Re: [PATCH v2 8/9] gdb/python: add gdbpy_dict_wrapper:allocate_dict helper

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

On 03/03/2026 18:30, Tom Tromey wrote:
>>>>>> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> Python extension objects that support __dict__ must inherit from
>> gdbpy_dict_wrapper, a wrapper class that stores the PyObject
>> corresponding to the __dict__ attribute.
> 
>> Currently, management of this dictionary is not centralized, and
>> each Python extension object implements its own logic to create,
>> access, and destroy it.
> 
>> This patch focuses on the allocation of the dictionary, introduces
>> a new helper function, gdbpy_dict_wrapper::allocate_dict(), and
>> adapts the existing code to use the helper.
> 
>> +  if (! gdbpy_dict_wrapper::allocate_dict (object))
>>       return nullptr;
> 
> gdb style doesn't put a space after '!' in an expression.
> There's a few instances of this.
> 

Fixed.

>> +
>> +  template <class T>
>> +  static bool
>> +  allocate_dict (gdbpy_ref<T>& self)
> 
> Space before "&"
> 
>> +  {
>> +    static_assert (std::is_base_of_v<gdbpy_dict_wrapper, T>
>> +		   || std::is_same_v<PyObject, T>);
>> +    auto *obj = static_cast<gdbpy_dict_wrapper *> (self.get ());
>> +    obj->dict = PyDict_New ();
>> +    return obj->dict != nullptr;
> 
> It seems to me that if this were a non-static method, then it also would
> not have to be a template method, like
> 
>      bool allocate_dict ()
>      {
>        // maybe?   gdb_assert (dict == nullptr);
>        dict = PyDict_New ();
>        return dict != nullptr;
>      }
> 
> The idea being that inheritance takes care of the details, so this
> works:
> 
>    gdbpy_ref<field_object> blah;
>    if (blah->allocate_dict ()) ...
> 
> Unless we have a case where this is needed for plain gdbpy_ref<>?
> But based on an earlier patch I would think that's not possible.
> 
> Tom

You are right, I needlessly overcomplicated things.
There is no case where this is needed for plain gdbpy_ref<>.
The approach with the inheritance is simpler and cleaner.

Regarding the assertion "gdb_assert (dict == nullptr);", it seems that PyObject_New() does not zero the memory so the assertion can be randomly triggered by garbage. I won't keep it.

Fixed in the next revision.

Matthieu