On Thu, Dec 18, 2014 at 5:04 AM, Aaron Ballman <aa...@aaronballman.com> wrote: > > On Wed, Dec 17, 2014 at 9:55 PM, Richard Smith <rich...@metafoo.co.uk> > wrote: > > On Mon, Dec 15, 2014 at 9:44 AM, Aaron Ballman <aa...@aaronballman.com> > > wrote: > >> > >> On Mon, Dec 15, 2014 at 12:26 PM, Joerg Sonnenberger > >> <jo...@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 > >> >> <jo...@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@cs.uiuc.edu > >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
