[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))
----------------
aaron.ballman wrote:
> 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.
> Sorry @serge-sans-paille for my think-o, Richard is right.
> 
> My thinking was "if the declaration can be found at the global namespace, then it's reserved", but.. that's a hypothetical declaration, not an actual one, so of course the name lookup won't find it. Derp.
> 
> I guess we really do have to look at the `DeclContext`.
> 
> @rsmith -- just to be clear, from http://eel.is/c++draft/lex.name#3.2, does "for use as a name in the global namespace" imply macros as well? e.g., can an implementation decide to use this reservation to `#define _foo 12`, which would cause problems with a `_foo` declared in a user-defined namespace? If so, then we can ignore what the declaration context is because the name could be snatched out from under the user via a macro anyway.
Regarding macros, I think it depends what you mean.

The implementation is permitted to declare additional `_foo` names in the global namespace, and I think the intent is that these can be either predeclared or declared in headers. The user is permitted to declare `_foo` names in other (non-reserved) namespaces.

As a result, neither party should define an `_foo` macro, because doing so would step on the other party's namespace (though it would be OK for user code to do so after all of their standard library includes, I suppose). Specifically, I think it would be non-conforming for the implementation to `#define` such a macro (because the name is not reserved to the implementation in all contexts), and I think it would be UB via [macro.names]/1 for user code to `#define` such a macro that actually collided with a name that happened to be declared in the global namespace by a standard library header.

We can probably warn on `#define _foo` if we want to, but I don't think we can produce a `-pedantic-error` for that case, because I think it's (sometimes) valid for user code to do that.


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

https://reviews.llvm.org/D93095