[Piglit] [PATCH] shader_runner: Cast isspace inputs to int to silence warnings.

[Jose Fonseca]

----- Original Message -----
> On Tue, Feb 21, 2012 at 7:09 AM, Brian Paul <brianp at vmware.com>
> wrote:
> > On 02/20/2012 09:31 PM, Kenneth Graunke wrote:
> >>
> >> On 02/19/2012 10:45 PM, Vinson Lee wrote:
> >>>
> >>> Fixes these GCC warnings on Cygwin.
> >>> shader_runner.c: In function ‘strcpy_to_space’:
> >>> shader_runner.c:202:2: warning: array subscript has type ‘char’
> >>> shader_runner.c: In function ‘eat_whitespace’:
> >>> shader_runner.c:216:2: warning: array subscript has type ‘char’
> >>> shader_runner.c: In function ‘eat_text’:
> >>> shader_runner.c:229:2: warning: array subscript has type ‘char’
> >>
> >>
> >> These are really bizarre warnings. We're not doing array
> >> subscripting
> >> here at all. We're calling isspace() on a char.
> >
> >
> > IIRC, some std C lib implementations use an array of true/false
> > values
> > indexed by character to implement isspace(), isalpha(), etc.  Maybe
> > that's
> > what's happening here. But still, it's a weird warning.
> 
> 
> This is the source from Cygwin.
> 
> /usr/include/ctypes.h
>     47  #ifndef __cplusplus
>     48  /* These macros are intentionally written in a manner that
>     will trigger
>     49     a gcc -Wall warning if the user mistakenly passes a 'char'
>     instead
>     50     of an int containing an 'unsigned char'.  Note that the
>     sizeof will
>     51     always be 1, which is what we want for mapping EOF to
> __ctype_ptr__[0];
>     52     the use of a raw index inside the sizeof triggers the gcc
>     warning if
>     53     __c was of type char, and sizeof masks side effects of the
>     extra __c.
>     54     Meanwhile, the real index to __ctype_ptr__+1 must be cast
>     to int,
>     55     since isalpha(0x100000001LL) must equal isalpha(1), rather
>     than being
>     56     an out-of-bounds reference on a 64-bit machine.  */
>     57  #define __ctype_lookup(__c)
> ((__ctype_ptr__+sizeof(""[__c]))[(int)(__c)])
>     58
>     59  #define isalpha(__c)    (__ctype_lookup(__c)&(_U|_L))
>     60  #define isupper(__c)    ((__ctype_lookup(__c)&(_U|_L))==_U)
>     61  #define islower(__c)    ((__ctype_lookup(__c)&(_U|_L))==_L)
>     62  #define isdigit(__c)    (__ctype_lookup(__c)&_N)
>     63  #define isxdigit(__c)   (__ctype_lookup(__c)&(_X|_N))
>     64  #define isspace(__c)    (__ctype_lookup(__c)&_S)
>     65  #define ispunct(__c)    (__ctype_lookup(__c)&_P)
>     66  #define isalnum(__c)    (__ctype_lookup(__c)&(_U|_L|_N))
>     67  #define isprint(__c)
>        (__ctype_lookup(__c)&(_P|_U|_L|_N|_B))
>     68  #define isgraph(__c)    (__ctype_lookup(__c)&(_P|_U|_L|_N))
>     69  #define iscntrl(__c)    (__ctype_lookup(__c)&_C)
> 
> 
> >
> >
> >
> >> This is perfectly legal, idiomatic, and common practice. Isn't the
> >> whole point of isspace() and friends to tell you what class a
> >> /character/ is in? Yes, they're specified with ints, but people
> >> use
> >> chars all the time. They're supposed to be silently promoted.
> >>
> >> IMO, if the Cygwin compiler can't handle isspace() on chars, it's
> >> broken. I would just turn off the warning.
> >>
> >> My vote on this patch (and the asmparsertest one) would be 'no'.
> >
> >
> > Vinson, do you know if there's a compiler switch to silence this
> > one?
> 
> From http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html.
> 
> <quote>
> -Wchar-subscripts
> Warn if an array subscript has type char. This is a common cause of
> error, as programmers often forget that this type is signed on some
> machines. This warning is enabled by -Wall.
> </quote>
> 
> I haven't tried it but -Wno-char-subscripts should be the compiler
> switch.
> 
> 
> I committed this patch before other people's feedback arrived. Please
> feel free to revert my changes.

It sounds like passing chars higher >= 0x80 (i.e, from -128 to -1) will give wrong results.

Still, I don't understand why cygwin decided to have this complex macro to spot an error, instead of of simply casting arguments to an int...

Do we really care about cygwin? I'd say that the best way of testing on windows would be using native windows binaries. I don't think that cygwin buys anything here.

Jose