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