Any feedback? Thanks, --Serge
2014-11-14 10:36 GMT+06:00 Serge Pavlov <sepavl...@gmail.com>: > 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 cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
