[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

serge-sans-paille marked an inline comment as done.
serge-sans-paille 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))
----------------
rsmith wrote:
> 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.
> If you're trying to detect whether the name is being introduced in the global namespace scope,
That's indeed the goal

> you'll need to look at the DeclContext of the declaration instead of just the identifier.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13640
 
+  warnOnReservedIdentifier(New);
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16296
   } else {
+    if (TUK == TUK_Definition)
+      warnOnReservedIdentifier(New);
----------------
rsmith wrote:
> Why do we not diagnose the other possible `TagUseKind`s? `struct _foo;` and `struct _foo *p;` both use reserved identifiers too.
We have a test case for `struct _foo` and its correctly diagnosed. I'll double check for pointer / reference too.


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

https://reviews.llvm.org/D93095