Re: [PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-03-23 2:11 p.m., Simon Marchi via Gdb-patches wrote:
> On 2021-03-23 2:07 p.m., Andreas Schwab wrote:> On Mär 23 2021, Simon Marchi via Gdb-patches wrote:
>>
>>> @@ -545,14 +540,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>>>  	  /* If we have our own "invoke" method, then allow unknown
>>>  	     sub-commands.  */
>>>  	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
>>> -	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
>>> +	  cmd = add_prefix_cmd (cmd_name.get (),
>>> +				(enum command_class) cmdtype,
>>>  				NULL, docstring, &obj->sub_list,
>>>  				pfx_name, allow_unknown, cmd_list);
>>>  	}
>>>        else
>>> -	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
>>> +	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
>>>  		       docstring, cmd_list);
>>>  
>>> +      /* The above doesn't copy nor take ownership of the name... so we just
>>> +         leak it.  */
>>
>> s/leak/release/?
> 
> I really wanted to say "leak", as we release it without an owner and
> accept it will never be freed.  Sometimes we release in order to
> transfer ownership to the callee (who hasn't been updated to use unique
> pointers yet), but that's not the case here.  I think the comment would
> be clear with either word.

Ah, I think I'm mistaken, the Python code uses:

    cmd->name_allocated = 1;

Which means the cmd_list_element would free the name if it ever gets
deleted.

I'll change the comment to:

      /* If successful, the above takes ownership of the name, since we set
         name_allocated, so release it.  */
      cmd_name.release ();

However, name_allocated isn't set in py-param.c, I guess it should...

Simon