[PATCH] D93095: Introduce -Wreserved-identifier

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:1097
+  // ignored values) that we don't warn on it.
+  if (Name.size() <= 1)
+    return ReservedIdentifierStatus::NotReserved;
----------------
serge-sans-paille wrote:
> rsmith wrote:
> > Would it make sense to move the rest of this function to a member on `IdentifierInfo`? That is, the rest of this function would become:
> > 
> > ```
> > ReservedIdentifierStatus Status = II->isReserved(LangOpts);
> > if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope &&
> >     isa<ParmVarDecl>(this) || isTemplateParameter() ||
> >     !getDeclContext()->getRedeclContext()->isTranslationUnit())
> >   return ReservedIdentifierStatus::NotReserved;
> > return Status;
> > ```
> > 
> > That way, we should be able to reuse the same implementation to detect reserved macros too.
> I did the `II->isReserved(LangOpts)` part, but the updated condition `!getDeclContext()->getRedeclContext()->isTranslationUnit()` fails the validation. I did already spend a lot of time getting one that catches the behavior we wanted, so 'm not super-eager to investigate why this change would not work ;-)
The current lexical context check does not look correct -- the lexical context is not relevant here, because the language rule is constrained by the semantic context. What test cases does the new condition fail?

Please also add something like the testcases from my other comment; I can't find any tests for friends or for local extern declarations in the current set of tests, and that's the kind of corner case that would be affected by this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93095/new/

https://reviews.llvm.org/D93095