Re: [Patch] New: CR16 port

["'Rask Ingemann Lambertsen'" <rask at sygehus dot dk>]

On Mon, Jul 16, 2007 at 05:51:49PM +0530, Pompapathi V Gadad wrote:
> Please review the patch and comment.

   This time around, I have only comments on the formatting. See also
<URL:http://www.gnu.org/prep/standards/standards.html.gz#Formatting> and
<URL:http://gcc.gnu.org/codingconventions.html>. I have not commented on
every place where the formatting doesn't follow the coding conventions, so
you should make a pass over the whole file, looking for the sort of things I
have commented on.

> Index: config/cr16/cr16.h
> ===================================================================
> --- config/cr16/cr16.h	(revision 0)
> +++ config/cr16/cr16.h	(revision 0)
...

> +#define FUNCTION_BOUNDARY   (!optimize_size? BIGGEST_ALIGNMENT : BITS_PER_WORD)
> +
> +/* Biggest alignment on CR16C+ is 32-bit as internal bus is AMBA based 
> + * where as CR16C is proprietary internal bus architecture. */
> +#define BIGGEST_ALIGNMENT   ((TARGET_CR16CP)?32:16)

   We want a space on both sides of ? and :.

> +/* Check if sizes are same and then check if it is possible to rename */
> +#define HARD_REGNO_RENAME_OK(SRC, DEST)                 \
> +    ( (   ((SRC >= CR16_FIRST_DWORD_REGISTER)           \
> +            && (DEST >= CR16_FIRST_DWORD_REGISTER))     \
> +       || ((SRC < CR16_FIRST_DWORD_REGISTER)            \
> +            && (DEST < CR16_FIRST_DWORD_REGISTER))      \
> +      )                                                 \
> +      ?!cr16_interrupt_function_p() || (df_regs_ever_live_p(DEST)) \
> +      : 0                                               \
> +    )

   We also want a space after the function name in function calls.

> +#define REGNO_OK_FOR_BASE_P(REGNO) \
> +  ((REGNO) < FIRST_PSEUDO_REGISTER \
> +   || (reg_renumber && (unsigned)reg_renumber[REGNO] < FIRST_PSEUDO_REGISTER))
> +
> +/* TODO: For now lets not support index addressing mode. */
> +#define REGNO_OK_FOR_INDEX_P(REGNO) \
> +  ( ((REGNO >= CR16_FIRST_DWORD_REGISTER) \
> +     && ((REGNO) < FIRST_PSEUDO_REGISTER)) \
> +   || (reg_renumber \
> +       && (   ((unsigned)reg_renumber[REGNO] >= CR16_FIRST_DWORD_REGISTER) \
> +       && ((unsigned)reg_renumber[REGNO] < FIRST_PSEUDO_REGISTER)))\
> +  )

   Spaces after type casts.

> +#define SIGNED_INT_FITS_N_BITS(imm, N) \
> +  ((((imm) < ((long long)1<<((N)-1))) && ((imm) >= -((long long)1<<((N)-1)))) ? 1 : 0)
> +
> +#define UNSIGNED_INT_FITS_N_BITS(imm, N) \
> +  (((imm) < ((long long)1<<(N)) && (imm) >= (long long)0) ? 1 : 0)

   Spaces around << and - also (except in e.g. foo = -bar;) and preferably
no more than 80 characters per line.

> Index: config/cr16/cr16.c
> ===================================================================
> --- config/cr16/cr16.c	(revision 0)
> +++ config/cr16/cr16.c	(revision 0)
...
> +      /* Now prepare the instruction operands */
> +      if ( word_cnt > 0)
...
> +      if (push_or_pop == 1)

and

> +          strcat(temp_str, one_inst_str);
> +
> +          /* Add the pop instruction list */
> +          strcat (return_str, temp_str);

   Inconsistent formatting.

> +  /* If there is no need in push and adjustment to sp, return. */

   Comments should be formatted with two spaces after each period.

-- 
Rask Ingemann Lambertsen