Re: [PATCH 2/6] Andes nds32: machine description of nds32 porting (1).

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Mon, 8 Jul 2013, Chung-Ju Wu wrote:

> +/* ------------------------------------------------------------------------ */
> +
> +/*++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++*
> + *|                                                                        |*
> + *| This file is divided into six parts:                                   |*
> + *|                                                                        |*
> + *|   PART 1: Auxiliary static function and variable declarations.         |*
> + *|                                                                        |*
> + *|   PART 2: Target hooks static function and variable declarations.      |*
> + *|                                                                        |*
> + *|   PART 3: Initialize target hooks structure and definitions.           |*
> + *|                                                                        |*
> + *|   PART 4: Implement auxiliary static function definitions,             |*
> + *|           the prototype is in nds32.c.                                 |*
> + *|                                                                        |*
> + *|   PART 5: Implement target hook stuff definitions,                     |*
> + *|           the prototype is in nds32.c.                                 |*
> + *|                                                                        |*
> + *|   PART 6: Implement extern function definitions,                       |*
> + *|           the prototype is in nds32-protos.h.                          |*
> + *|                                                                        |*
> + *++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++*/

Fancy comment formatting like this is not the norm in GCC.  Comments 
should just look like

/* Comment text.
   Subsequent lines lined up like this.

   End of comment.  */

I also advise topologically sorting static functions and variables and 
putting the target hook structure initialization at the end of the file, 
to minimize the number of static declarations needed separate from the 
definitions (they should generally be needed only for functions called 
recursively).

> +/* Define intrinsic register names.
> +   Please refer to nds32_intrinsic.h file, the index is corresponding to
> +   'enum nds32_intrinsic_registers' data type values.
> +   NOTE that the base value starting from 1024.  */
> +static const char* nds32_intrinsic_register_names[] =

I suspect the array here can itself be made const.

> +/* 17.8 Register Classes */

Please avoid all comments referring to section numbers in particular 
versions of the manual; there's no way those comments are ever going to 
get updated when sections are added, removed, renamed or reordered.

> +  /* Prepare jmptbl section and symbol name.  */
> +  sprintf (section_name, ".nds32_jmptbl.%02d", vector_id);
> +  sprintf (symbol_name, "_nds32_jmptbl_%02d", vector_id);

Anywhere you use sprintf, please use snprintf instead (with the size 
argument generally being sizeof (buffer_variable), as good coding practice 
to avoid buffer overruns if there was a mistake in the reasoning that the 
buffer size was big enough.

> +  /* Currently, we don't support PIC code generation yet.  */
> +  if (flag_pic)
> +    error ("not support -fpic");

Something not supported yet sounds like a case for sorry () instead of 
error ().

> +/* For the following option pairs:
> +     1. (-mbig-endian, -mlittle-endian)
> +     2. (-mreduced-regs, -mfull-regs)
> +   We reject them to appear in the command line.  */
> +#define CC1_SPEC                                                              \
> +  " %{mbig-endian:"                                                           \
> +  "   %{mlittle-endian:%emay not use both -mbig-endian and -mlittle-endian}}" \
> +  " %{mreduced-regs:"                                                         \
> +  "   %{mfull-regs:%emay not use both -mreduced-regs and -mfull-regs}}"

If you put these options in Negative() loops in the .opt file, then you 
can have the last option on the command line winning (with only the last 
option being visible to specs), which is the usual practice, instead of 
having such errors.

> +#define LINK_SPEC \
> +  " %{v} %{mbig-endian:-EB} %{mlittle-endian:-EL}"

Do you really need %{v} here?  Such things shouldn't generally be 
architecture-specific.

-- 
Joseph S. Myers
joseph@codesourcery.com