[PATCH] D93095: Introduce -Wreserved-identifier

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

rsmith added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:2421-2428
+    // Perform a lookup at TUScope. If it succeeds, we're at global scope and a
+    // single '_' is enough to be reserved.
+    LookupResult IdentifierLookup(*this, II, SourceLocation(),
+                                  Sema::LookupAnyName,
+                                  Sema::ForExternalRedeclaration);
+    IdentifierLookup.suppressDiagnostics();
+    if (LookupName(IdentifierLookup, TUScope))
----------------
I don't understand why name lookup is involved here. Whether an identifier is reserved doesn't depend on whether it's already been declared in the global namespace or not. It's valid to declare `_foo` in some user-defined namespace regardless of whether there's already a `_foo` in the global namespace, and it's invalid to declare `_foo` in the global namespace regardless of whether there's already a `_foo` in the global namespace.

If you're trying to detect whether the name is being introduced in the global namespace scope, per C++ [lex.name]/3.2, you can't do that like this; you'll need to look at the `DeclContext` of the declaration instead of just the identifier.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:5898
 
+  if (D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_extern)
+    warnOnReservedIdentifier(New);
----------------
Why does the presence / absence of `extern` matter here?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13640
 
+  warnOnReservedIdentifier(New);
+
----------------
Is there somewhere more central you can do this, rather than repeating it once for each kind of declaration? (Eg, `PushOnScopeChains`)


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16296
   } else {
+    if (TUK == TUK_Definition)
+      warnOnReservedIdentifier(New);
----------------
Why do we not diagnose the other possible `TagUseKind`s? `struct _foo;` and `struct _foo *p;` both use reserved identifiers too.


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

https://reviews.llvm.org/D93095