[PATCH] D115248: [Clang] Fix PR28101

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3430
           << D.getName().TemplateId->LAngleLoc;
+      if (cast<CXXRecordDecl>(CurContext)->getDeclName() == Name)
+        Diag(Loc, diag::err_member_name_of_class) << Name;
----------------
rZhBoYao wrote:
> erichkeane wrote:
> > I see we are diagnosing this ONLY inside of the case where it is a template.  Where is this caught when we are in the non-template case, why doesn't THAT catch these cases, and what would it take to do so?
> CheckCompletedCXXClass should catch diag::err_member_name_of_class. However, these FieldDecl have no name hence uncaught. Maybe we should set the identifier for this declarator here.
I'm not sure what the side effect of that is... Can you give it a shot?


================
Comment at: clang/test/SemaCXX/PR28101.cpp:8
+
+#ifdef CASE_1
+
----------------
rZhBoYao wrote:
> erichkeane wrote:
> > This isn't necessary, the compiler in verify mode should see all errors.  I see above at most 2 invocations of the compiler as necessary.
> Instantiation of each of the 4 classes could crash on its own, so I thought I'd separate them into 4 invocations.
They shouldn't crash afterwards, right?  So this shouldn't be a problem.


================
Comment at: clang/test/SemaCXX/PR28101.cpp:16
+
+A<int> instantiate() { return {nullptr}; }
+
----------------
rZhBoYao wrote:
> erichkeane wrote:
> > How come there are no notes in this test that say 'in instantiation of...'?  I would expect those in at least some of these cases, right?
> Because it was caught before instantiation and when instantiating the FieldDecl is gone after calling `D.setInvalidType();`. If I set the identifier for the FieldDecl and let `CheckCompletedCXXClass` handle the "has the same name as its class" error, there will be the "in instantiation of..." note.
I think we definitely need to do so there, the instantiation trace not being here makes this a much less useful diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115248