This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 2/6] Andes nds32: machine description of nds32 porting (1).
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Chung-Ju Wu <jasonwucj at gmail dot com>
- Cc: gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 9 Jul 2013 23:20:46 +0000
- Subject: Re: [PATCH 2/6] Andes nds32: machine description of nds32 porting (1).
- References: <CADj25HMFJUOHWC5ZvDOrJH+xkCoN8sxHXhD2vb36bM95irWEdg at mail dot gmail 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