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

Tue Dec 23 15:08:35 PST 2014

----- Original Message -----
> From: "Richard Smith" <richard at metafoo.co.uk>
> To: "Aaron Ballman" <aaron at aaronballman.com>
> Cc: "Joerg Sonnenberger" <joerg at britannica.bec.de>, "llvm cfe" <cfe-commits at cs.uiuc.edu>
> Sent: Thursday, December 18, 2014 4:06:38 PM
> Subject: Re: r224012 - Emit warning if define or undef reserved identifier or	keyword.
> 
> 
> On Thu, Dec 18, 2014 at 5:04 AM, Aaron Ballman <
> aaron at aaronballman.com > wrote:
> 
> 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,
> 
> 
> I find it hard to believe that extern and static are sometimes
> disabled for debug build purposes, since doing so is likely to
> completely break the program. "#define inline" will also typically
> break the program due to multiple-definition errors.
> 
> 
> 
> I can understand "#define const /**/" for compilers that don't
> support const, but Clang is not such a compiler, so code that does
> that and is built with Clang seems to deserve a warning.
> 

I've certainly seen plenty of code that "#define restrict /**/" when not compiling in C99 mode. That should probably not get a warning (unless we want to suggest using __restrict__ instead).

 -Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory