Thank you for review!
================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:299
@@ +298,3 @@
+def warn_pp_defundef_reserved_ident : Warning<
+ "reserved identifier is used as macro name">, DefaultIgnore,
+ InGroup<ReservedIdAsMacro>;
----------------
rnk wrote:
> 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?
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.
================
Comment at: lib/Basic/IdentifierTable.cpp:216
@@ +215,3 @@
+static KeywordStatus GetKeywordStatus(const LangOptions &LangOpts,
+ tok::TokenKind K) {
+ switch (K) {
----------------
rnk wrote:
> indentation is off
Fixed.
================
Comment at: lib/Lex/PPDirectives.cpp:111
@@ +110,3 @@
+
+static MacroDiag ShouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
+ StringRef Text = II->getName();
----------------
rnk wrote:
> The LLVM naming conventions call for leading lower camel case function names,
> so this should be `shouldWarnOnMacroDef`.
Ah, old naming. Fixed.
================
Comment at: lib/Lex/PPDirectives.cpp:117
@@ +116,3 @@
+ if (Text.size() >= 2 && Text[0] == '_' &&
+ ((Text[1] >= 'A' && Text[1] <= 'Z') || Text[1] == '_'))
+ return MD_WarnStrict;
----------------
rnk wrote:
> rnk wrote:
> > indentation
> 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.
Introduced function isReservedId, which makes this check.
================
Comment at: lib/Lex/PPDirectives.cpp:133
@@ +132,3 @@
+
+static MacroDiag ShouldWarnOnMacroUndef(Preprocessor &PP, IdentifierInfo *II) {
+ if (II->isKeyword(PP.getLangOpts()))
----------------
rnk wrote:
> s/Should/should/
Fixed.
================
Comment at: lib/Lex/PPDirectives.cpp:143
@@ +142,3 @@
+ if (Text.size() >= 2 && Text[0] == '_' &&
+ ((Text[1] >= 'A' && Text[1] <= 'Z') || Text[1] == '_'))
+ return MD_WarnStrict;
----------------
rnk wrote:
> Reuse this code
Done: calls isReservedId.
================
Comment at: lib/Lex/PPDirectives.cpp:197
@@ +196,3 @@
+ PresumedLoc PLoc = getSourceManager().getPresumedLoc(MacroNameLoc, false);
+ WarnOnReserved = (strcmp(PLoc.getFilename(), "<built-in>") != 0);
+ }
----------------
rnk wrote:
> There's got to be a better way to identify builtin macros...
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".
http://reviews.llvm.org/D6194
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
