Re: [PATCH] Change management of tdesc_arch_data

[Simon Marchi <simark@simark.ca>]

On 2020-07-27 3:36 p.m., Tom Tromey wrote:
> While working on something else, I noticed that tdesc_data_cleanup
> took a void* parameter.  Looking more into this, I found that
> tdesc_use_registers expected a transfer of ownership.
> 
> I think it's better to express this sort of thing via the type system,
> when possible.  This patch changes tdesc_data_alloc to return a unique
> pointer, changes tdesc_use_registers to accept an rvalue reference,
> and then adapts all the users.
> 
> Note that a deleter structure is introduced to avoid having to move
> tdesc_arch_data to the header file.

Good idea.

> @@ -784,10 +784,8 @@ tdesc_data_alloc (void)
>     architecture).  */
>  
>  void
> -tdesc_data_cleanup (void *data_untyped)
> +tdesc_arch_data_deleter::operator() (struct tdesc_arch_data *data) const

I think the comment above this is not relevant, and should be made into the
usual /* See foo.h. */.

> @@ -1097,7 +1095,7 @@ set_tdesc_pseudo_register_reggroup_p
>  void
>  tdesc_use_registers (struct gdbarch *gdbarch,
>  		     const struct target_desc *target_desc,
> -		     struct tdesc_arch_data *early_data,
> +		     tdesc_arch_data_up &&early_data,
>  		     tdesc_unknown_register_ftype unk_reg_cb)
>  {
>    int num_regs = gdbarch_num_regs (gdbarch);
> @@ -1112,7 +1110,6 @@ tdesc_use_registers (struct gdbarch *gdbarch,
>  
>    data = (struct tdesc_arch_data *) gdbarch_data (gdbarch, tdesc_data);
>    data->arch_regs = early_data->arch_regs;

Not really important, but this could be an std::move to avoid copying the vector.

And since the function now takes an rvalue reference, we know it's fine to do this.
> diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
> index 57945d21db7..b78db89b4e2 100644
> --- a/gdb/tic6x-tdep.c
> +++ b/gdb/tic6x-tdep.c
> @@ -1140,7 +1140,7 @@ tic6x_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
>    struct gdbarch *gdbarch;
>    struct gdbarch_tdep *tdep;
> -  struct tdesc_arch_data *tdesc_data = NULL;
> +  tdesc_arch_data_up tdesc_data;

Move this where this is used, in the if?  That will avoid unnecessary construction/destruction.

That would apply to other arches as well.

Simon