Re: [PATCH] Add support for the __flash qualifier on AVR

[Joel Brobecker <brobecker at adacore dot com>]

Hello,

> 2014-07-08  Pierre Langlois  <pierre.langlois@embecosm.com>
> 
> gdb/
> 	* avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
> 	(AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
> 	(avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
> 	(avr_pointer_to_address): Likewise.
> 	(avr_address_class_type_flags): New function.
> 	(avr_address_class_type_flags_to_name): Likewise.
> 	(avr_address_class_name_to_type_flags): Likewise.
> 	(avr_gdbarch_init): Set address_class_type_flags,
> 	address_class_type_flags_to_name and
> 	address_class_name_to_type_flags.
> 
> gdb/testsuite/
> 	* gdb.arch/avr-flash-qualifer.c: New.
> 	* gdb.arch/avr-flash-qualifer.exp: New.

Sorry about the delay in reviewing this. Some comments below.

> +/* Map DW_AT_address_class attributes to a type_instance_flag_value.  Note that
> +   this attribute is only valid with pointer types and therefore the flag is set
> +   to the pointer type and not its target type.  */

Can you start the comment by saying that this function implements the
address_class_name_to_type_flags gdbarch method?  You can then add
the comment above as a second paragraph, or perhaps as an implementation
comment inside the function body itself.

> +static int
> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
> +{

Please add a comment saying that this function implements the
address_class_type_flags gdbarch method. Generally speaking, all
new functions without exception should have an introductory comment.

> +  // __flash qualifier

Please do not use C++-style comments for the moment.

> +  if (dwarf2_addr_class == 1 && byte_size == 2)
> +    return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
> +  return 0;
> +}
> +
> +/* Convert a type_instance_flag_value to an address space qualifier and
> +   vice-versa.  */
> +
> +static const char*
> +avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)

Same as above regarding the function's introductory comment. And also,
I'd prefer it if we avoided two functions being documented with
the same comment, as this opens the door for a function becoming
undocumented if the two functions somehow get separated.

> +{
> +  if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH)
> +    return "flash";
> +  else
> +    return NULL;
> +}
> +
> +static int
> +avr_address_class_name_to_type_flags (struct gdbarch *gdbarch,
> +                                      const char* name,
> +                                      int *type_flags_ptr)

Missing introductory comment ("Implements the ...").

-- 
Joel