[PATCH] D93095: Introduce -Wreserved-identifier

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:377
+  "%0 is a reserved identifier">,
+  InGroup<ReservedIdentifier>, DefaultIgnore;
+
----------------
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).


================
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
----------------
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.)


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

https://reviews.llvm.org/D93095