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

[Richard Smith]

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?

> (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).


> #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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141217/04710f2c/attachment.html>