[PATCH] D80295: [Sema] Diagnose more cases of static data members in local or unnamed classes

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks, looks great.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:6904
             << Name << RD->getTagKind();
           Invalid = true;
+        } else if (RD->isLocalClass()) {
----------------
john.brawn wrote:
> rjmccall wrote:
> > john.brawn wrote:
> > > rjmccall wrote:
> > > > This diagnostic actually ignores the tag kind that passed down to it, which should be fixed.  Also, you should pass in the tag kind for the actual anonymous class you found.
> > > > 
> > > > While I'm looking at this code: `isLocalClass` is mis-designed and doesn't work for any of our non-`FunctionDecl` local contexts.  This check should be `if (RD->getParentFunctionOrMethod())`.
> > > > This diagnostic actually ignores the tag kind that passed down to it, which should be fixed. Also, you should pass in the tag kind for the actual anonymous class you found.
> > > 
> > > Will do.
> > > 
> > > > While I'm looking at this code: isLocalClass is mis-designed and doesn't work for any of our non-FunctionDecl local contexts. This check should be if (RD->getParentFunctionOrMethod()).
> > > 
> > > CXXMethodDecl has FunctionDecl as a parent class, so isLocalClass will find and return a CXXMethodDecl. Checking it on
> > > 
> > > ```
> > > class Example {
> > >   void method() {
> > >     class X {
> > >       static int x;
> > >     };
> > >   }
> > > };
> > > ```
> > > I get the error as expected. Do you have an example where it doesn't work?
> > > Will do.
> > 
> > You need to also update the diagnostic to actually care about this.
> > 
> > >  Do you have an example where it doesn't work?
> > 
> > All of the standard C/C++ local contexts are FunctionDecls, but "blocks", ObjC methods, and OpenMP captured statements aren't.  The simplest example would be (under `-fblocks`):
> > 
> > ```
> > void test() {
> >   ^{
> >     class X {
> >       static int x;
> >     };
> >   }();
> > }
> > ```
> > You need to also update the diagnostic to actually care about this.
> 
> It looks like it already does? E.g. in the tests added to anonymous-struct.cpp where the error is "anonymous struct" or "anonymous class" depending on if it's an anonymous class or struct.
Oh, sorry, I just misread the diagnostic.


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

https://reviews.llvm.org/D80295