<div dir="ltr">Any feedback?<div class="gmail_extra"><br clear="all"><div><div class="gmail_signature">Thanks,<br>--Serge<br></div></div>
<br><div class="gmail_quote">2014-11-14 10:36 GMT+06:00 Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thank you for review!<br>
<span class=""><br>
================<br>
Comment at: include/clang/Basic/DiagnosticLexKinds.td:299<br>
@@ +298,3 @@<br>
+def warn_pp_defundef_reserved_ident : Warning<<br>
+  "reserved identifier is used as macro name">, DefaultIgnore,<br>
+  InGroup<ReservedIdAsMacro>;<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> 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.<br>
><br>
> 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?<br>
</span>Implemented two groups, -Wkeyword-macro and -Wreserved-id-macro, the first is on by default.<br>
Bug tracker of gcc has issue [[ <a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51437" target="_blank">https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51437</a> ]], which is a clone of clang PR11488. It is still in unconfirmed state. Experiments show that gcc does not emit warnings for described use.<br>
<span class=""><br>
================<br>
Comment at: lib/Basic/IdentifierTable.cpp:216<br>
@@ +215,3 @@<br>
+static KeywordStatus GetKeywordStatus(const LangOptions &LangOpts,<br>
+  tok::TokenKind K) {<br>
+  switch (K) {<br>
----------------<br>
</span>rnk wrote:<br>
> indentation is off<br>
Fixed.<br>
<span class=""><br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:111<br>
@@ +110,3 @@<br>
+<br>
+static MacroDiag ShouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {<br>
+  StringRef Text = II->getName();<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> The LLVM naming conventions call for leading lower camel case function names, so this should be `shouldWarnOnMacroDef`.<br>
</span>Ah, old naming. Fixed.<br>
<span class=""><br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:117<br>
@@ +116,3 @@<br>
+  if (Text.size() >= 2 && Text[0] == '_' &&<br>
+    ((Text[1] >= 'A' && Text[1] <= 'Z') || Text[1] == '_'))<br>
+    return MD_WarnStrict;<br>
----------------<br>
rnk wrote:<br>
> rnk wrote:<br>
> > indentation<br>
> 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.<br>
</span>Introduced function isReservedId, which makes this check.<br>
<span class=""><br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:133<br>
@@ +132,3 @@<br>
+<br>
+static MacroDiag ShouldWarnOnMacroUndef(Preprocessor &PP, IdentifierInfo *II) {<br>
+  if (II->isKeyword(PP.getLangOpts()))<br>
----------------<br>
</span>rnk wrote:<br>
> s/Should/should/<br>
Fixed.<br>
<span class=""><br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:143<br>
@@ +142,3 @@<br>
+  if (Text.size() >= 2 && Text[0] == '_' &&<br>
+    ((Text[1] >= 'A' && Text[1] <= 'Z') || Text[1] == '_'))<br>
+    return MD_WarnStrict;<br>
----------------<br>
</span>rnk wrote:<br>
> Reuse this code<br>
Done: calls isReservedId.<br>
<span class=""><br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:197<br>
@@ +196,3 @@<br>
+    PresumedLoc PLoc = getSourceManager().getPresumedLoc(MacroNameLoc, false);<br>
+    WarnOnReserved = (strcmp(PLoc.getFilename(), "<built-in>") != 0);<br>
+  }<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> There's got to be a better way to identify builtin macros...<br>
</span>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".<br>
<br>
<a href="http://reviews.llvm.org/D6194" target="_blank">http://reviews.llvm.org/D6194</a><br>
<br>
<br>
</blockquote></div><br></div></div>