[PATCH] D93095: Introduce -Wreserved-identifier

Wed Feb 3 15:42:21 PST 2021

rsmith added inline comments.

================
Comment at: clang/lib/AST/Decl.cpp:1087
+  StringRef Name = II->getName();
+  // '_' is a reserved identifier, but it's use is so common (e.g. to store
+  // ignored values) that we don't warn on it.
----------------

================
Comment at: clang/lib/AST/Decl.cpp:1096
+    // Walk up the lexical parents to determine if we're at TU level or not.
+    const DeclContext * DC = getLexicalDeclContext();
+    while(DC->isTransparentContext())
----------------
aaron.ballman wrote:
> You may want to run the patch through clang-format.
The lexical parent doesn't matter here; what we care about is whether this declaration would conflict with a declaration in the global namespace, which means we should check the semantic parent. So we want `getDeclContext()->getRedeclContext()->isTranslationUnit()`.

For a variable or function, you should also check `isExternC()`, because `extern "C"` functions and variables (declared in any scope) conflict with variables and functions with the same name declared in the global namespace scope.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:5557-5558
+    return;
+  if (!Context.getSourceManager().isInSystemHeader(D->getLocation()) &&
+      D->isReserved(getLangOpts()))
+    Diag(D->getLocation(), diag::warn_reserved_identifier) << D;
----------------
Swap the order of these checks; the "is reserved" check is faster and will usually allow us to short-circuit, whereas we're probably usually not in a system header and that check involves nontrivial work recursively decomposing the given source location.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:17114

+    warnOnReservedIdentifier(FD);
+
----------------
It would be more consistent with the other calls to this function to call this when we create each individual field, not when we finalize the record definition.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:13640

+  warnOnReservedIdentifier(New);
+
----------------
serge-sans-paille wrote:
> serge-sans-paille wrote:
> > 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.
> I tried PushOnScopeChains, and this does not capture all the required declarations. I failed to find another place :-/
What cases is it missing? Can we add calls to `PushOnScopeChains` to those cases (perhaps with a new flag to say "don't actually push it into scope"?) I'm not super happy about adding a new thing that all places that create declarations and add them to scope need to remember to do, to drive this warning. Cases are going to get forgotten that way.

================
Comment at: clang/lib/Sema/SemaTemplate.cpp:1680-1681

+  for (NamedDecl *P : Params)
+    warnOnReservedIdentifier(P);
+
----------------
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.

================
Comment at: clang/test/Sema/reserved-identifier.c:13
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =               // expected-warning {{'__1' is a reserved identifier}}
----------------
It'd be useful to test that we don't diagnose

```
void foo(unsigned int _not_reserved) { ... }
```

================
Comment at: clang/test/Sema/reserved-identifier.c:31
+struct _Zebulon; // expected-warning {{'_Zebulon' is a reserved identifier}}
+struct _Zebulon2 {}* p; // expected-warning {{'_Zebulon2' is a reserved identifier}}
+
----------------
You don't seem to have a test for `TUK_Reference`:

```
struct _Zebulon3 *p;
```

(Nor for `TUK_Friend`.)

================
Comment at: clang/test/Sema/reserved-identifier.c:49-50
+// FIXME: According to clang declaration context layering, _preserved belongs to
+// the translation unit, so we emit a warning. It's unclear that's what the
+// standard mandate, but it's such a corner case we can live with it.
+void func(struct _preserved { int a; } r) {} // expected-warning {{'_preserved' is a reserved identifier}}
----------------
aaron.ballman wrote:
> I think we're correct to warn on this. Because the type specifier appears within the parameter list, it has function prototype scope per C2x 6.2.1p4. However, the identifier `_preserved` is put into the struct name space, which hits the "in the tag name spaces" part of: "All identifiers that begin with an underscore are reserved for use as identifiers with file scope in both the ordinary and tag name spaces".
`_preserved` is not an identifier with file scope, so I think we shouldn't warn here. Perhaps the problem is that we're doing this check before the struct gets reparented into the function declaration (which only happens after we finish parsing all the parameters and build the function declaration).

We could perhaps check the `Scope` in addition to checking the `DeclContext` to detect such cases.

================
Comment at: clang/test/Sema/reserved-identifier.c:53
+
+extern char *_strdup(const char *); // expected-warning {{'_strdup' is a reserved identifier}}
+
----------------
aaron.ballman wrote:
> This is a case I don't think we should warn on. As some examples showing this sort of thing in the wild:
> 
> https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/lz.cpp
> https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/svc1.cpp
You mean, specifically for `_strdup`? In general, this looks exactly the like the kind of declaration we'd want to warn on. If we don't want a warning here, we could perhaps recognize `_strdup` as a builtin lib function. (I think they shouldn't warn, because we'll inject a builtin declaration, so this would be a redeclaration. We should test that!)

================
Comment at: clang/test/Sema/reserved-identifier.cpp:21
+
+template <class __> // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
----------------
Please also test the `_not_reserved` case here. Parameters are weird, because they're parsed before we have a `DeclContext` created to correspond with their scope, so they start off with the translation unit as their parent.

================
Comment at: clang/test/Sema/reserved-identifier.cpp:63
+static union {
+  int _barbeFleurie; // no-warning
+};
----------------
This should warn: this declaration would conflict with an `int _barbeFleurie();` in a system header.

... although weirdly Clang doesn't diagnose that conflict. Hm. That looks like a bug.

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

https://reviews.llvm.org/D93095