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

[Aaron Ballman]

On Wed, Dec 17, 2014 at 9:55 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Mon, Dec 15, 2014 at 9:44 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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.
>
>
> Can you explain here on the mailing list why this seemingly-arbitrary
> whitelist is a good idea?

These specific keywords are sometimes disabled for debug build
purposes, and Joerg had made a compelling argument that you should not
have to disable a specific warning just to get debug builds to work.
When discussed, his feeling was that these keywords were the only ones
compelling or reasonable enough to disable.

I'm not (personally) strongly tied to the ability to effectively
disable keywords, but since it sounds like at least one sizable code
base relies on such behavior, it seems a reasonable suggestion.

>
>> > (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)
>
>
> I disagree; the right thing to check is that the macro name and macro
> expansion are equivalent tokens. If we support an inline and __inline
> keyword, it's fine to define one as the other, this us just a special case
> of (1).

Agreed -- that's a much better heuristic.

~Aaron

>
>>
>> #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
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits