Explicit casts in ctype.h suppress compiler warnings

Explicit casts in ctype.h suppress compiler warnings

[Alexander Monakov]

For the following erroneous source code:

#include <ctype.h>
int f(char *c)
{
  return isdigit(c) || isspace(c);
}

GCC warns only for passing a pointer to isspace; isdigit is implemented as a
macro that casts its argument to unsigned, and the warning is suppresed
because the origin of the cast is in a system header.  Since isspace is
implemented with a static inline helper function, there is a warning.  With
glibc headers, no warning is issued in either case for a similar reason.

I think it would be nice if musl's ctype.h could aid the compiler in
diagnosing erroneous use, like it happens today for only for isspace() of all
macros declared there.  The cost of restructuring the header to achieve that
does not seem too high.  Thoughts?

Thanks.
Alexander

Re: Explicit casts in ctype.h suppress compiler warnings

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

Am Freitag, den 17.04.2015, 13:59 +0300 schrieb Alexander Monakov:
> For the following erroneous source code:
> 
> #include <ctype.h>
> int f(char *c)
> {
>   return isdigit(c) || isspace(c);
> }
> 
> GCC warns only for passing a pointer to isspace; isdigit is implemented as a
> macro that casts its argument to unsigned, and the warning is suppresed
> because the origin of the cast is in a system header.  Since isspace is
> implemented with a static inline helper function, there is a warning.  With
> glibc headers, no warning is issued in either case for a similar reason.

I generally think that casts are a bad idea, anyhow, and should only
be used where it must be done, that is basically for pointer to
integer conversion (and back). Code like this

#define isdigit(a) (((unsigned)(a)-'0') < 10)

can easily be replaced by

#define isdigit(a) (((unsigned const){a}-'0') < 10)

to change the explicit conversion to an implicit one in the
initializer of the compound literal. Then, any compiler would have to
diagnose if "a" would be a pointer.

Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Explicit casts in ctype.h suppress compiler warnings

[Rich Felker]

On Fri, Apr 17, 2015 at 01:59:14PM +0300, Alexander Monakov wrote:
> For the following erroneous source code:
> 
> #include <ctype.h>
> int f(char *c)
> {
>   return isdigit(c) || isspace(c);
> }
> 
> GCC warns only for passing a pointer to isspace; isdigit is implemented as a
> macro that casts its argument to unsigned, and the warning is suppresed
> because the origin of the cast is in a system header.  Since isspace is
> implemented with a static inline helper function, there is a warning.  With
> glibc headers, no warning is issued in either case for a similar reason.
> 
> I think it would be nice if musl's ctype.h could aid the compiler in
> diagnosing erroneous use, like it happens today for only for isspace() of all
> macros declared there.  The cost of restructuring the header to achieve that
> does not seem too high.  Thoughts?

Do you have an idea in mind for how we could achieve that? I suspect
the macros are still better optimizable than the inline function
approach, so I'd lean towards doing a macro that avoids evaluating c
and just checks its type, which would involve using ?: I think.

Rich

Re: Explicit casts in ctype.h suppress compiler warnings

[Rich Felker]

On Fri, Apr 17, 2015 at 06:49:54PM +0200, Jens Gustedt wrote:
> Am Freitag, den 17.04.2015, 13:59 +0300 schrieb Alexander Monakov:
> > For the following erroneous source code:
> > 
> > #include <ctype.h>
> > int f(char *c)
> > {
> >   return isdigit(c) || isspace(c);
> > }
> > 
> > GCC warns only for passing a pointer to isspace; isdigit is implemented as a
> > macro that casts its argument to unsigned, and the warning is suppresed
> > because the origin of the cast is in a system header.  Since isspace is
> > implemented with a static inline helper function, there is a warning.  With
> > glibc headers, no warning is issued in either case for a similar reason.
> 
> I generally think that casts are a bad idea, anyhow, and should only
> be used where it must be done, that is basically for pointer to
> integer conversion (and back). Code like this
> 
> #define isdigit(a) (((unsigned)(a)-'0') < 10)
> 
> can easily be replaced by
> 
> #define isdigit(a) (((unsigned const){a}-'0') < 10)
> 
> to change the explicit conversion to an implicit one in the
> initializer of the compound literal. Then, any compiler would have to
> diagnose if "a" would be a pointer.

In another place (math.h) I removed this type of compound literal
usage because it was incompatible with C++, but the macros are
suppressed in C++ anyway. Still they might break -pedantic with
-std=c89. I do like this approach best in principle if it works
though, because the rules for when an error occurs are basically the
same as the rules for a real function.

Rich

Re: Explicit casts in ctype.h suppress compiler warnings

[Alexander Monakov]

> In another place (math.h) I removed this type of compound literal
> usage because it was incompatible with C++, but the macros are
> suppressed in C++ anyway. Still they might break -pedantic with
> -std=c89. I do like this approach best in principle if it works
> though, because the rules for when an error occurs are basically the
> same as the rules for a real function.

I confirm that the idea works, and as Rich said it causes a warning with
-pedantic -std=c89 with gcc-4.5..4.7 (but not 4.8, 4.9).

> Do you have an idea in mind for how we could achieve that? I suspect
> the macros are still better optimizable than the inline function
> approach, so I'd lean towards doing a macro that avoids evaluating c
> and just checks its type, which would involve using ?: I think.

I admit I was thinking of doing isspace-style inlines everywhere, but thanks
to your suggestion I was able to come up with this:

static __inline void __is_int(int a) {}
#define isdigit(a) (__is_int(0?(a):0), ((unsigned)(a)-'0') < 10)

Actually, thinking about GCC behavior a bit more, I was lucky that there's a
warning for isspace in the first place: wrong argument to __isspace originates
in the context of a system-header-declared macro, so generally I'd say that
the same warning-suppression logic should have applied.

Alexander

Re: Explicit casts in ctype.h suppress compiler warnings

[Szabolcs Nagy]

* Alexander Monakov <amonakov@ispras.ru> [2015-04-17 21:24:09 +0300]:
> 
> I confirm that the idea works, and as Rich said it causes a warning with
> -pedantic -std=c89 with gcc-4.5..4.7 (but not 4.8, 4.9).
> 

i assume we could use compound literals in c89 with __extension__

> > Do you have an idea in mind for how we could achieve that? I suspect
> > the macros are still better optimizable than the inline function
> > approach, so I'd lean towards doing a macro that avoids evaluating c
> > and just checks its type, which would involve using ?: I think.
> 
> I admit I was thinking of doing isspace-style inlines everywhere, but thanks
> to your suggestion I was able to come up with this:
> 
> static __inline void __is_int(int a) {}
> #define isdigit(a) (__is_int(0?(a):0), ((unsigned)(a)-'0') < 10)
> 

i think using :0 there is not useful because null
pointer constants are special in ?:

i think rich meant something like

#define isdigit(a) (((0?1U:(a))-'0') < 10)

which works when the conversion rank of a is <= unsigned

Re: Explicit casts in ctype.h suppress compiler warnings

[William Ahern]

On Fri, Apr 17, 2015 at 12:52:38PM -0400, Rich Felker wrote:
> On Fri, Apr 17, 2015 at 06:49:54PM +0200, Jens Gustedt wrote:
<snip>
> > I generally think that casts are a bad idea, anyhow, and should only
> > be used where it must be done, that is basically for pointer to
> > integer conversion (and back). Code like this
> > 
> > #define isdigit(a) (((unsigned)(a)-'0') < 10)
> > 
> > can easily be replaced by
> > 
> > #define isdigit(a) (((unsigned const){a}-'0') < 10)
> > 
> > to change the explicit conversion to an implicit one in the
> > initializer of the compound literal. Then, any compiler would have to
> > diagnose if "a" would be a pointer.
> 
> In another place (math.h) I removed this type of compound literal
> usage because it was incompatible with C++, but the macros are
> suppressed in C++ anyway. Still they might break -pedantic with
> -std=c89. I do like this approach best in principle if it works
> though, because the rules for when an error occurs are basically the
> same as the rules for a real function.

I think C++11 brace initialization

	int { 42 }

works nearly identically to C99 compound literal definition

	(int){ 42 }

The big difference is that the lifetime of C++ temporaries have expression
scope, whereas compound literals have block scope. But that's irrelevant
where the values will be copied and no pointer is derived.*

A simple macro could be used to select the syntax. However, I don't think
something like `unsigned char { 42 }' will work. The type name needs to be a
single identifier, so a typedef would be needed: u_char { 42 }.

* Unfortunately, by default g++ accepts C99-style compound literals, but for
whatever reason gives them expression-scoped lifetimes as-if they were C++
temporaries. This gave me and some other people grief when when using
pointers to compound literals, either explicitly or implicitly through
array-to-pointer decay). See Bug #53220. Everything seemed to work when
compiled as C++, unless you were fortunate enough to get a crash near the
offending code. More recent versions of g++ now at least warn when a pointer
is derived from a compound literal.

I think clang++ has the same behavior.

Re: Explicit casts in ctype.h suppress compiler warnings

[Szabolcs Nagy]

* Szabolcs Nagy <nsz@port70.net> [2015-04-17 20:35:12 +0200]:
> > I admit I was thinking of doing isspace-style inlines everywhere, but thanks
> > to your suggestion I was able to come up with this:
> > 
> > static __inline void __is_int(int a) {}
> > #define isdigit(a) (__is_int(0?(a):0), ((unsigned)(a)-'0') < 10)
> > 
> 
> i think using :0 there is not useful because null
> pointer constants are special in ?:
> 

ah i see what you are doing there now

meanwhile i tested that (__extension__ (unsigned){a})
is accepted without warning in c89 mode

Re: Explicit casts in ctype.h suppress compiler warnings

[Rich Felker]

On Fri, Apr 17, 2015 at 09:43:24PM +0200, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2015-04-17 20:35:12 +0200]:
> > > I admit I was thinking of doing isspace-style inlines everywhere, but thanks
> > > to your suggestion I was able to come up with this:
> > > 
> > > static __inline void __is_int(int a) {}
> > > #define isdigit(a) (__is_int(0?(a):0), ((unsigned)(a)-'0') < 10)
> > > 
> > 
> > i think using :0 there is not useful because null
> > pointer constants are special in ?:
> > 
> 
> ah i see what you are doing there now

Yes, this is exactly the type of thing I hoped could be done.

> meanwhile i tested that (__extension__ (unsigned){a})
> is accepted without warning in c89 mode

Try again with -pedantic.

Rich

Re: Explicit casts in ctype.h suppress compiler warnings

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2015-04-17 16:21:26 -0400]:
> On Fri, Apr 17, 2015 at 09:43:24PM +0200, Szabolcs Nagy wrote:
> > meanwhile i tested that (__extension__ (unsigned){a})
> > is accepted without warning in c89 mode
> 
> Try again with -pedantic.
> 

__extension__ silences pedantic warnings

i tried various gcc/clang versions with c89/c++98
and it seems to work

http://goo.gl/lWP02j

(__extension__ should be __GNUC__ conditional but
otherwise it seems to me a reasonable solution)

Re: Explicit casts in ctype.h suppress compiler warnings

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

Am Freitag, den 17.04.2015, 20:35 +0200 schrieb Szabolcs Nagy:
> #define isdigit(a) (((0?1U:(a))-'0') < 10)
> 
> which works when the conversion rank of a is <= unsigned

but not if it has conversion rank larger then unsigned and the result
of the substraction has a negative result. So what you think of

#define isdigit(a) (((0?1ULL:(a))-'0') < 10ULL)

This ensures that the type of the left is always ullong, so the
substraction overflows to something big if a is not a digit.

Jens


-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Explicit casts in ctype.h suppress compiler warnings

[Rich Felker]

On Fri, Apr 17, 2015 at 09:24:09PM +0300, Alexander Monakov wrote:
> > Do you have an idea in mind for how we could achieve that? I suspect
> > the macros are still better optimizable than the inline function
> > approach, so I'd lean towards doing a macro that avoids evaluating c
> > and just checks its type, which would involve using ?: I think.
> 
> I admit I was thinking of doing isspace-style inlines everywhere, but thanks
> to your suggestion I was able to come up with this:
> 
> static __inline void __is_int(int a) {}
> #define isdigit(a) (__is_int(0?(a):0), ((unsigned)(a)-'0') < 10)

This still depends on the inline function getting honored, and puts
extra crap in the debug output. Instead, how about:

#define isdigit(a) (0 ? (isdigit)(a) : ((unsigned)(a)-'0') < 10)

Rich

Re: Explicit casts in ctype.h suppress compiler warnings

[Morten Welinder]

Ideally isspace and friends should also cause a warning
if called with a "char" or "signed char" argument.  Such calls
are generally wrong unless the range of the argument
can somehow be controlled.  ("char" might be unsigned,
for example.)

The number of people who have no idea why they should
not make such calls is astounding.  The typical response
is to blame the compiler and add an (int) cast.  The search
at

    https://searchcode.com/?q=isspace+%28int%29

claims ~55k instances.  Ignore the first page which is finding
isspace implementations, but incorrect uses.

M.






On Fri, Apr 17, 2015 at 5:36 PM, Rich Felker <dalias@libc.org> wrote:
> On Fri, Apr 17, 2015 at 09:24:09PM +0300, Alexander Monakov wrote:
>> > Do you have an idea in mind for how we could achieve that? I suspect
>> > the macros are still better optimizable than the inline function
>> > approach, so I'd lean towards doing a macro that avoids evaluating c
>> > and just checks its type, which would involve using ?: I think.
>>
>> I admit I was thinking of doing isspace-style inlines everywhere, but thanks
>> to your suggestion I was able to come up with this:
>>
>> static __inline void __is_int(int a) {}
>> #define isdigit(a) (__is_int(0?(a):0), ((unsigned)(a)-'0') < 10)
>
> This still depends on the inline function getting honored, and puts
> extra crap in the debug output. Instead, how about:
>
> #define isdigit(a) (0 ? (isdigit)(a) : ((unsigned)(a)-'0') < 10)
>
> Rich