aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:377
+ "%0 is a reserved identifier">,
+ InGroup<ReservedIdentifier>, DefaultIgnore;
+
----------------
Quuxplusone wrote:
> If you leave it like this, you //will// receive multiple bug reports per year
> about how in some contexts the compiler diagnoses `_X` and `_x` equally, and
> in other contexts the compiler diagnoses `_X` but fails to diagnose `_x`.
>
> You should make two diagnostics: `-Wreserved-extern-identifier` and
> `-Wreserved-pp-identifier` (or something along those lines), and their
> messages should be carefully worded to explain //why// the warning is
> happening — "identifier %0 is reserved for use at file scope," "identifier %0
> is reserved in all contexts," etc.
>
> Btw, the reason some identifiers (like `_X`) are reserved in all contexts is
> so that the implementation can define them as macros (e.g. header guards).
I think splitting the diagnostic into two groups could be useful, but I'd still
like the groups to be under a common grouping so I can enable/disable all
reserved identifier warnings at once.
As for adding the extra rationale to the diagnostic message: I think if we want
to add extra wording, it should be more about how the user can fix the name and
less about why the standard reserves something. e.g., `"identifier %0 is
reserved because %select{it starts with '__'|it contains '__'|it is the same
identifier as a keyword|etc}0"`. Knowing that something is reserved at file
scope is somewhat helpful, but knowing how to change the name to appease the
compiler is too.
================
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())
----------------
You may want to run the patch through clang-format.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:5554
+ // Avoid warning twice on the same identifier, and don't warn on
redeclaration
+ // of system decl
+ if (D->getPreviousDecl())
----------------
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10018
if (DeclaratorDecl *DD = dyn_cast<DeclaratorDecl>(D)) {
- for (unsigned i = 0; i < DD->getNumTemplateParameterLists(); ++i)
+ for (unsigned i = 0; i < DD->getNumTemplateParameterLists(); ++i) {
ParameterLists.push_back(DD->getTemplateParameterList(i));
----------------
Unrelated changes?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:544
+ if (!Context.getSourceManager().isInSystemHeader(IdentLoc) &&
+ TheDecl->isReserved(getLangOpts()))
----------------
Can you not call `warnOnReservedIdentifier(TheDecl)` in this case?
================
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}}
----------------
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".
================
Comment at: clang/test/Sema/reserved-identifier.c:53
+
+extern char *_strdup(const char *); // expected-warning {{'_strdup' is a
reserved identifier}}
+
----------------
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
================
Comment at: clang/test/Sema/reserved-identifier.c:9
+int _foo() { return 0; } // expected-warning {{'_foo' is a reserved
identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
----------------
serge-sans-paille wrote:
> Quuxplusone wrote:
> > aaron.ballman wrote:
> > > Can you add a test that we do not warn on an extern declaration? e.g.,
> > > ```
> > > extern char *_strdup(const char *);
> > > ```
> > > This is sometimes used (esp in older C code bases) to avoid having to
> > > include an entire header to scrape one declaration out of it, and there
> > > are popular libraries (like the MSVC CRT) that have APIs starting with a
> > > single underscore and lowercase letter.
> > >
> > > The idea here is: if it's an extern declaration, the identifier is
> > > "owned" by some other declaration and this is not likely something the
> > > user has control over.
> > Should that logic also apply to a forward declaration like `struct _foo;`?
> > Should it apply to `struct _foo { int i; };`? These are also things the
> > user might not have control over. (And they're equally things that the user
> > //could// pull out into a separate .h file surrounded by disabled-warning
> > pragmas, if they really wanted to.)
> I'd say 100% yeah to the forward declaration, but I'm unsure about the actual
> definition, because there's no way to distinguish it from a user definition.
> Should that logic also apply to a forward declaration like `struct _foo;`?
I could see a case for not warning on a forward declaration like that, but I
think it's a bit less compelling because I can't seem to find that pattern
happening in the wild like I can for extern function declarations.
> Should it apply to struct _foo { int i; };?
This is a definition of the type and so I think it should be warned on (unless
it's in a system header).
================
Comment at: clang/test/Sema/reserved-identifier.cpp:77
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
----------------
Quuxplusone wrote:
> This should get a warning, since it's using an identifier "reserved for any
> use."
> https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier
> http://eel.is/c++draft/lex.name#3.1
> If the implementation predefines `#define _BarbeBleue 42` (which the
> implementation is permitted to do), then this code won't compile.
+1, this really is reserved.
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
