[PATCH] D93095: Introduce -Wreserved-identifier

Mon Feb 1 09:51:12 PST 2021

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