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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
