[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:1099
+    const DeclContext *CurrentContext = getDeclContext();
+    while (true) {
+      if (isa<TranslationUnitDecl>(CurrentContext))
----------------
Rather than trying to manually decide whether the name will be exposed at the top level, I wonder if we should use `Sema::LookupName()` to try to find the name in the global scope. That would move this code from `AST` into `Sema` to avoid layering issues, so it'd be something more like `bool Sema::IsDeclReserved(const NamedDecl *ND) const` or `bool Sema::IsIdentifierReserved(const IdentifierInfo *II) const`.

The idea is to perform the lookup in global scope and if the lookup finds the identifier, then regardless of how it got there (anonymous union field, anonymous namespaces, enumeration constants, etc), we know it may conflict with an external declaration and diagnose.

WDYT?


================
Comment at: clang/test/Sema/reserved-identifier.cpp:40
+  return foo__bar(); // no-warning
+}
----------------
hubert.reinterpretcast wrote:
> serge-sans-paille wrote:
> > aaron.ballman wrote:
> > > 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.
> > User defined literal tested below and behave as expected.
> @aaron.ballman, the "not reserved" comment re: `_X` for the literal operator using the `operator string-literal identifier` form above seems suspect to me. See `_Bq` example in [over.literal].
Thanks, @hubert.reinterpretcast, you're correct -- I forgot just how hard we made the rules for UDLs when we decided to require a leading underscore. :-(


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

https://reviews.llvm.org/D93095