[PATCH] D93095: Introduce -Wreserved-identifier

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 6 09:22:49 PST 2021


aaron.ballman added inline comments.


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


================
Comment at: clang/test/Sema/reserved-identifier.c:24
+  _Other,     // expected-warning {{'_Other' is a reserved identifier}}
+  _other      // no-warning
+};
----------------
I'm on the fence about whether this should have no warning or not. Enumeration constants in C (and sometimes in C++, depending on the enumeration) are in the enclosing scope. e.g.,
```
enum foo {
  _bar
};

int i = _bar;
```
So if a user has an enumeration constant named `_bar`, the implementation is not free to add `int _bar(void);` as it will cause compile errors. WDYT?


================
Comment at: clang/test/Sema/reserved-identifier.cpp:58
+// we skip this one because it's not at top-level.
+int _barbatruc; // no-warning
+}
----------------
This is another case that I'm on the fence about failing to warn on because the name `_barbatruc` would conflict with a name introduced by the implementation. Another interesting variant of the same problem, for C++:
```
static union {
  int _field;
};
```
I wonder if the rule should not be whether the declaration is at file scope or not, but whether the declared identifier can be found at file scope or not?


================
Comment at: clang/test/Sema/reserved-identifier.cpp:40
+  return foo__bar(); // no-warning
+}
----------------
aaron.ballman wrote:
> You should also have some tests for:
> ```
> template <typename T>
> void _Foobar(); // Even though it's not instantiated, it's still reserved.
> 
> template <typename _Ty> // Reserved
> void whatever();
> 
> void func() {
>   int array[10];
>   for (auto _A : array) // Reserved
>     ;
> }
> 
> class _C { // Reserved
> public:
>   _C(); // Not reserved
> };
> 
> unsigned operator "" huttah(unsigned long long); // Reserved (http://eel.is/c++draft/usrlit.suffix#1)
> 
> unsigned operator "" _W(unsigned long long); // Reserved
> unsigned operator "" _w(unsigned long long); // Reserved
> 
> static unsigned operator "" _X(unsigned long long); // Not reserved
> static unsigned operator "" _x(unsigned long long); // Not reserved
> ```
I think some of these tests are still missing. I'm especially worried about the user-defined literal cases being diagnosed, as we'd be warning to not do the exact thing users are expected to do.


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

https://reviews.llvm.org/D93095



More information about the cfe-commits mailing list