This is an archive of the discontinued LLVM Phabricator instance.

Emit warning if define or undef reserved identifier or keyword.
ClosedPublic

Authored by sepavloff on Nov 10 2014, 9:54 AM.

Details

Summary

This change implements warnings if macro name is identical to a keyword or
reserved identifier. The warnings are different depending on the "danger"
of the operation. Defining macro that replaces a keyword is on by default.
Other cases produce warning that is off by default but can be turned on
using option -Wreserved-id-macro.

This change fixes PR11488.

Diff Detail

Event Timeline

sepavloff updated this revision to Diff 15994.Nov 10 2014, 9:54 AM
sepavloff retitled this revision from to Emit warning if define or undef reserved identifier or keyword..
sepavloff updated this object.
sepavloff edited the test plan for this revision. (Show Details)
sepavloff added a subscriber: Unknown Object (MLST).
rnk added a subscriber: rnk.Nov 10 2014, 11:08 AM
rnk added inline comments.
include/clang/Basic/DiagnosticLexKinds.td
299

Personally, I think it's confusing to have a warning group where some instances are on by default and others are off. We end up with a tri-state of default, on, off. Many users do things like clang -w -Werror -Wthing1 -Wthing2 -Wthing3 ... to explicitly use a curated set of warnings. It would be too bad if they couldn't use the on-by-default part of this warning due to lumping them into the same group.

Maybe we can split the group into -Wkeyword-macro and -Wreserved-id-macro? Does gcc have a flag name we should be using for compatibility?

lib/Basic/IdentifierTable.cpp
216

indentation is off

lib/Lex/PPDirectives.cpp
111

The LLVM naming conventions call for leading lower camel case function names, so this should be shouldWarnOnMacroDef.

117

indentation

117

We have a table lookup based isUppercase() for this. Factoring this check out as a helper gives us a good place to put this quote from the C++ standard.

133

s/Should/should/

143

Reuse this code

197

There's got to be a better way to identify builtin macros...

sepavloff updated this revision to Diff 16192.Nov 13 2014, 8:10 PM

Addressed reviewer's notes.

Thank you for review!

include/clang/Basic/DiagnosticLexKinds.td
299

Implemented two groups, -Wkeyword-macro and -Wreserved-id-macro, the first is on by default.
Bug tracker of gcc has issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51437, which is a clone of clang PR11488. It is still in unconfirmed state. Experiments show that gcc does not emit warnings for described use.

lib/Basic/IdentifierTable.cpp
216

Fixed.

lib/Lex/PPDirectives.cpp
111

Ah, old naming. Fixed.

117

Introduced function isReservedId, which makes this check.

133

Fixed.

143

Done: calls isReservedId.

197

It seems they are identified by similar way throughout clang sources. SourceManager considers all sources as files and the only way to check if SourceLocation is in some predefined source is to check "file name".

rnk accepted this revision.Dec 1 2014, 2:09 PM
rnk added a reviewer: rnk.

According to phabricator, lib/Lex/PPDirectives.cpp got chmoded to executable. Looks good if you can revert that change.

This revision is now accepted and ready to land.Dec 1 2014, 2:09 PM
sepavloff closed this revision.Dec 2 2014, 3:06 AM
sepavloff updated this revision to Diff 16804.

Closed by commit rL223114 (authored by @sepavloff).