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