[PATCH] Emit warning if define or undef reserved identifier or keyword.

[Serge Pavlov]

Any feedback?

Thanks,
--Serge

2014-11-14 10:36 GMT+06:00 Serge Pavlov <sepavloff at 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141125/d04aedac/attachment.html>