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

[Serge Pavlov]

In r224512 implementation changed in accordance with the recommendations.
Thanks to all for feedback.

The solution still lacks support of an important case. As were noted, the
pattern:

    #define inline

is used in configuration scripts. It may convert inline function to global
functions, linker will report duplicate definitions in this case. The
reason may be obscure for a user, because source code is OK. Finding the
place where the definition takes place also may be not simple. A warning
that helps discover the point where a keyword was hidden could be a great
help. Compiling PHP sources is a real world example of such use case.

We could use warning -Wreserved-id-macro for macros that redefine a keyword
allowed for configuration scripts, or introduce new warning group, which is
turned off by default.


Thanks,
--Serge

2014-12-18 8:55 GMT+06:00 Richard Smith <richard at metafoo.co.uk>:
>
> 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/20141218/37e4810c/attachment.html>