[PATCH] D93095: Introduce -Wreserved-identifier

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

rsmith 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|"
----------------
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.)


================
Comment at: clang/lib/AST/Decl.cpp:1084
+  const IdentifierInfo *II = nullptr;
+  if (const auto *FD = dyn_cast<FunctionDecl>(this))
+    II = FD->getLiteralIdentifier();
----------------
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.


================
Comment at: clang/lib/AST/Decl.cpp:1097
+  // ignored values) that we don't warn on it.
+  if (Name.size() <= 1)
+    return ReservedIdentifierStatus::NotReserved;
----------------
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.


================
Comment at: clang/lib/AST/Decl.cpp:1113-1119
+    if (!isa<ParmVarDecl>(this) && !isTemplateParameter()) {
+      const DeclContext *DC = getLexicalDeclContext();
+      while (DC->isTransparentContext())
+        DC = DC->getLexicalParent();
+      if (isa<TranslationUnitDecl>(DC))
+        return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+    }
----------------
This can be simplified a bit; I suggested edits above. Also, we want the semantic parent here, not the lexical parent, in order to warn on cases like

```
struct A {
  friend struct _b; // warning, starts with underscore at global scope
  friend void _f(); // warning, starts with underscore at global scope
};
void g() {
  void _h(); // warning, starts with underscore at global scope
  extern int _n; // warning, starts with underscore at global scope
}
```


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

https://reviews.llvm.org/D93095