[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:383
+  "identifier %0 is reserved because %select{"
+  "|" // passing 0 for %1 is not valid but we need that | to make the enum order match the diagnostic
+  "it starts with '_' at global scope|"
----------------
rsmith wrote:
> This is hopefully more useful to future readers and also terser. (I like including <ERROR> in the diagnostic in the bad case because if it ever happens we're more likely to get a bug report that way.)
Ooh, I like that suggestion, thanks!


================
Comment at: clang/lib/AST/Decl.cpp:1096
+
+    // Walk up the lexical parents to determine if we're at TU level or not.
+    if (isa<ParmVarDecl>(this) || isTemplateParameter())
----------------
This comment should be updated -- we no longer walk the lexical parents.


================
Comment at: clang/lib/AST/Decl.cpp:1084
+  const IdentifierInfo *II = nullptr;
+  if (const auto *FD = dyn_cast<FunctionDecl>(this))
+    II = FD->getLiteralIdentifier();
----------------
rsmith wrote:
> Please reorder the literal operator case after the plain `getIdentifier` case. I think this'd be more readable if the common and expected case is first, and it doesn't make any functional difference since a literal operator doesn't have a "regular" identifier name.
It looks like this code still needs to move.


================
Comment at: clang/lib/Basic/IdentifierTable.cpp:297
+
+    return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+  }
----------------
I think comments here may help -- we don't know that the identifier is at global scope within this call, so the return value looks a bit strange. This is a case where the enumerator name is a bit awkward in this context but makes a lot of sense in the context of the check.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4051-4052
   if (const auto *Id = CalleeDecl->getIdentifier())
-    if (Id->isReservedName())
+    if (Id->isReserved(CGM.getLangOpts()) !=
+        ReservedIdentifierStatus::NotReserved)
       return;
----------------
There's a slight functionality change here in that the code used to allow identifiers that start with a single underscore followed by a lowercase letter and that now early returns. Is that expected (and tested)?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:5564-5565
+  ReservedIdentifierStatus Status = D->isReserved(getLangOpts());
+  if (Status != ReservedIdentifierStatus::NotReserved)
+    if (Context.getSourceManager().isInMainFile(D->getLocation()))
+      Diag(D->getLocation(), diag::warn_reserved_extern_symbol) << D << Status;
----------------



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10998
   PushDeclContext(NamespcScope, Namespc);
+
   return Namespc;
----------------
Spurious whitespace change.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12673
   ActOnDocumentableDecl(NewND);
+
   return NewND;
----------------
Spurious whitespace change.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16743
 
+  warnOnReservedIdentifier(ND);
+
----------------
Doesn't `PushOnScopeChains()` do this already?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:545
+  if (!Context.getSourceManager().isInSystemHeader(IdentLoc)) {
+    ReservedIdentifierStatus Status = TheDecl->isReserved(getLangOpts());
+    if (Status != ReservedIdentifierStatus::NotReserved)
----------------
aaron.ballman wrote:
> This check should be reversed as well.
This still needs to be reversed so we check for system headers after checking the reserved status.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:1680-1681
 
+  for (NamedDecl *P : Params)
+    warnOnReservedIdentifier(P);
+
----------------
rsmith wrote:
> Again, it'd be more consistent to do this when we finish creating the declaration and push it into scope, for all kinds of declaration.
Is this handled by `PushOnScopeChains()`?


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

https://reviews.llvm.org/D93095