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

[Richard Smith]

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.

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