[PATCH] D86765: [clang] Don't emit "no member" diagnostic if the lookup fails on an invalid record decl.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:2584
+    // Don't diagnose problems with invalid record decl, the no_member
+    // diagnostic is likely bogus, e.g. if a base specificer is invalid, the
+    // derived class is invalid, and has no base class attached, lookup of base
----------------
sammccall wrote:
> I found the wording of this example a bit hard to follow.
> 
> What about "e.g. if a class is invalid because it's derived from an invalid base class, then missing members were likely supposed to be inherited".
> 
> I'm actually not convinced this is merely an example - is there any other reason for invalidity other than invalid base class where this logic applies?
> I'm actually not convinced this is merely an example - is there any other reason for invalidity other than invalid base class where this logic applies?

I suppose there are other examples, but failed to find one. 
Yeah, there are other reason that can cause an invalid RecordDecl (e.g. an invalid field), but it doesn't run into this code path. This code seems to be applied during template instantiation, I think it should be fine.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:2588
+    if (const auto *CD = dyn_cast<CXXRecordDecl>(DC))
+      if (CD->isInvalidDecl())
+        return ExprError();
----------------
sammccall wrote:
> this is a fairly general check, but the reasoning really only applies to invalid bases.
> If there's an easy way to check for that specifically instead, that'd be nice, but fine if not.
we don't record that the base specifier is not attached due to the invalidity, we could introduce a bit to do that, not sure it is worth for this particular case, might be worth to reconsider if we see large regressions (currently, we don't have any regressions in existing tests). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86765