[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:82
+enum ReservedIdentifierReason {
+  StartsWithUnderscoreAtGlobalScope,
+  StartsWithDoubleUnderscore,
----------------
serge-sans-paille wrote:
> aaron.ballman wrote:
> > Because this is not a scoped enum, should these enumerators get an `RIR_` prefix added to them?
> I used a class enum with a stream operator instead. Not a big fan of prefixed enum ;-)
Totally fine by me, thank you!


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:383
+  "identifier %0 is reserved because %select{"
+  "|"
+  "it starts with '_' at global scope|"
----------------
You may want to add a comment here to alert people to the fact that passing 0 for %1 is not valid but the `|` exists to make the enum order match the diagnostic. Otherwise it looks like it's possible to get a diagnostic `identifier 'whatever' is reserved because `.


================
Comment at: clang/lib/AST/Decl.cpp:1105-1111
+    if (Name[1] == '_') {
+      return ReservedIdentifierStatus::StartsWithDoubleUnderscore;
+    }
+    if ('A' <= Name[1] && Name[1] <= 'Z') {
+      return ReservedIdentifierStatus::
+          StartsWithUnderscoreFollowedByCapitalLetter;
+    }
----------------



================
Comment at: clang/lib/AST/Decl.cpp:1118-1120
+      if (isa<TranslationUnitDecl>(DC)) {
+        return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+      }
----------------



================
Comment at: clang/lib/AST/Decl.cpp:1125-1127
+  if (LangOpts.CPlusPlus && Name.contains("__")) {
+    return ReservedIdentifierStatus::ContainsDoubleUnderscore;
+  }
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:5554
+  // Avoid warning twice on the same identifier, and don't warn on redeclaration
+  // of system decl
+  if (D->getPreviousDecl())
----------------
aaron.ballman wrote:
> 
Still missing a full stop at the end of the comment here.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:5557-5558
+    return;
+  if (!Context.getSourceManager().isInSystemHeader(D->getLocation()) &&
+      D->isReserved(getLangOpts()))
+    Diag(D->getLocation(), diag::warn_reserved_identifier) << D;
----------------
rsmith wrote:
> Swap the order of these checks; the "is reserved" check is faster and will usually allow us to short-circuit, whereas we're probably usually not in a system header and that check involves nontrivial work recursively decomposing the given source location.
Richard's comment is still unaddressed as well.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13640
 
+  warnOnReservedIdentifier(New);
+
----------------
rsmith wrote:
> serge-sans-paille wrote:
> > serge-sans-paille wrote:
> > > rsmith wrote:
> > > > Is there somewhere more central you can do this, rather than repeating it once for each kind of declaration? (Eg, `PushOnScopeChains`)
> > > That would be sane. I'll check that.
> > I tried PushOnScopeChains, and this does not capture all the required declarations. I failed to find another place :-/
> What cases is it missing? Can we add calls to `PushOnScopeChains` to those cases (perhaps with a new flag to say "don't actually push it into scope"?) I'm not super happy about adding a new thing that all places that create declarations and add them to scope need to remember to do, to drive this warning. Cases are going to get forgotten that way.
I'm still wondering about this as well.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:545
+  if (!Context.getSourceManager().isInSystemHeader(IdentLoc)) {
+    ReservedIdentifierStatus Status = TheDecl->isReserved(getLangOpts());
+    if (Status != ReservedIdentifierStatus::NotReserved)
----------------
This check should be reversed as well.


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

https://reviews.llvm.org/D93095