r224012 - Emit warning if define or undef reserved identifier or keyword.

[Aaron Ballman]

On Mon, Dec 15, 2014 at 12:26 PM, Joerg Sonnenberger
<joerg at britannica.bec.de> wrote:
> On Mon, Dec 15, 2014 at 11:47:23AM -0500, Aaron Ballman wrote:
>> On Mon, Dec 15, 2014 at 11:35 AM, Joerg Sonnenberger
>> <joerg at britannica.bec.de> wrote:
>> > On Mon, Dec 15, 2014 at 11:32:02AM -0500, Aaron Ballman wrote:
>> >> On Sat, Dec 13, 2014 at 2:46 PM, Joerg Sonnenberger
>> >> > (2) #define restrict __restrict__ --> code that has to deal with pre-C99
>> >> > default in GCC.
>> >>
>> >> I'm not certain this should have triggered the warning in the first
>> >> place. Either restrict is a keyword (at which point, the #define
>> >> should be considered possibly harmful), or restrict isn't a keyword
>> >> (and then there's no reason for the warning in the first place).
>> >
>> > It happens in code that has to deal with different compilers. Consider a
>> > pre-generated config.h. In NetBSD we have to deal with different GCC
>> > versions and Clang, at least the former doesn't default to C99 :(
>>
>> I'm confused. Are you talking about vendors where restrict is a
>> keyword, but it's not implemented (properly)?
>
> configure will not pick it up for GCC with the C90 default.

Thank you for the clarification (here and on IRC), that makes a lot
more sense to me now.

>
>> >> > (3) #define extern, #define static --> debug, code sharing
>> >>
>> >> I think these should be a warning. If you're doing a debug build where
>> >> you want to turn off keywords, you can add a -Wno-keyword-macro flag
>> >> to your command line.
>> >
>> > I disagree. Having to change the warning settings just to get a debug
>> > build to work is stupid.
>>
>> Redefining keywords to be empty macros in production code is equally stupid. ;-)
>>
>> > My question remains: what bugs has this warning caught? I've heard that
>> > Microsoft believes this to be reasonable to warn about, but that's about
>> > it.
>>
>> CERT also feels this is reasonable to diagnose:
>> ...
>
> Well, keep in mind that C and restricted interface contracts don't
> necessarily agree with each other, so often various tricks are played.
> So summarize, I think the following should be accepted silently by
> default:
>
> (1) macro name equals macro body.

Agreed.

> (2) macro name is a "storage" class keyword (extern, static, inline) and
> the body is empty.
> (3) macro name is const.

I think this makes sense, but as an explicit whitelist of: static,
extern, inline, and const.

> (4) macro body wrapped with __ equals macro name.

Agreed, but slightly more relaxed -- a macro body prefixed with __
equals macro name, optionally with a suffixed with __. Eg)

#define inline __inline__ // Ok
#define inline __inline // Ok
#define inline // Ok
#define inline inline // Ok
#define inline not_inline // Not Ok
#define inline inline__ // Not Ok

~Aaron