Re: [gfortran,patch] PR fortran/27715

[Thomas Koenig <Thomas dot Koenig at online dot de>]

:REVIEWMAIL:

Hi FX,

I couldn't comment on the PR yesterday, hence my comments here :-)

On Mon, May 29, 2006 at 03:09:17PM +0200, François-Xavier Coudert wrote:
> The attached patch is a fix for PR fortran/27715, which basically is
> because string comparisons in the front-end are done by comparing
> chars (which are signed chars for example on i686-linux), and not
> unsigned chars. This is not especially bad in itself (although it's
> not the "usual" ordering for strings), but it conflicts with the way
> the library does comparisons, by memcmp, that is comparing unsigned
> chars.

The current situation also has the additional problem of an out-of-bounds
reference for machines with signed char (because it would reference
ascii_table[-1] for char 0xFF).  Definitely bad.

> 
> Regtested on i686-linux, OK for mainline and 4.1?
> 
> FX
> 
> :ADDPATCH fortran:


> Index: simplify.c
> ===================================================================
> --- simplify.c	(revision 113849)
> +++ simplify.c	(working copy)
> @@ -67,7 +67,7 @@
  
> -static int xascii_table[256];
> +static unsigned int xascii_table[256];

Why this change?  int is large enough to hold the relevant chars.

>  
>  
>  /* Range checks an expression node.  If all goes well, returns the
> @@ -1196,7 +1196,7 @@
>        return &gfc_bad_expr;
>      }
>  
> -  index = xascii_table[(int) e->value.character.string[0] & 0xFF];
> +  index = xascii_table[(unsigned char) e->value.character.string[0] & 0xFF];
>  
>    result = gfc_int_expr (index);
>    result->where = e->where;
> @@ -4016,9 +4016,9 @@
>  /* Given a collating table, create the inverse table.  */
>  
>  static void
> -invert_table (const int *table, int *xtable)
> +invert_table (const unsigned char *table, unsigned int *xtable)
>  {
> -  int i;
> +  unsigned int i;

Also unnecessary.  Unsigned loop counters have problems in general
(OK, not in this case :-), but an int would do fine here.

> -  int len, alen, blen, i, ac, bc;
> +  int len, alen, blen, i;
> +  unsigned char ac, bc;

In general, I would avoid doing char arithmetic, because it tends
to be less efficient.  Doing the cast to unsigned char where
ac and bc are assigned to would be fine.

Otherwise OK.

	Thomas