Re: [PATCH 02/16] gdb: convert address_class_type_flags_to_name to address_class_id_to_name

>>>>> Tankut Baris Aktemur <[email protected]> writes:

> In type instance flags, two bits are allocated for encoding the
> address class.  Although defined like a bitmask, those two bits in
> fact represent an architecture-specific enum value.  As a step towards
> making this conceptual separation clear, refactor the gdbarch method
> 'address_class_type_flags_to_name'.  This method is used for returning
> the name for the address class id encoded in type instance flags.
> Make this clear by passing it the address class id, instead of the
> whole flags.

I have two small suggestions.

> +ft32_address_class_id_to_name (struct gdbarch *gdbarch,
> +			       unsigned int address_class)
>  {
> -  if (type_flags & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1)
> +  if (address_class == 1)

First, I just wanted to point this out explicitly: normally I'd ding
this for using a hard-coded constant; but in this case another function
in the same file does essentially the same, and anyway this is a tdep
file that hasn't perhaps had a non-refactoring patch since 2017.  So I
think this can slide.

>  Method(
>      type="const char *",
> -    name="address_class_type_flags_to_name",
> -    params=[("type_instance_flags", "type_flags")],
> +    name="address_class_id_to_name",
> +    params=[("unsigned int", "address_class")],
>      predicate=True,
>  )
 
I think this is a good opportunity to add a comment to describe the
method.

> -  else if ((space_flag & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
> -	   && gdbarch_address_class_type_flags_to_name_p (gdbarch))
> -    return gdbarch_address_class_type_flags_to_name (gdbarch, space_flag);
> +
> +  unsigned int aclass = TYPE_ADDRESS_CLASS_FROM_INSTANCE_FLAGS (space_flag);
> +
> +  if (aclass != 0
> +      && gdbarch_address_class_id_to_name_p (gdbarch))
> +    return gdbarch_address_class_id_to_name (gdbarch, aclass);

Here I think the code could check gdbarch_address_class_id_to_name_p and
then unconditionally call it with whatever the value happens to be --
IMO we can assume that the arch methods handle 0.

thanks,
Tom