Re: [PATCH] Change management of tdesc_arch_data

Simon Marchi
2020-07-28T13:41:06Z

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