sammccall accepted this revision.
sammccall added a comment.

This seems like fixing a fairly specific case of a general problem: if the base 
class is invalid I would expect to see similar problems in other places (e.g. 
unqualified lookup from inside the class).
But it fixes a diagnostic the libc++ folks think is important...



================
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
----------------
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?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:2586
+    // derived class is invalid, and has no base class attached, lookup of base
+    // class members will fails.
+    if (const auto *CD = dyn_cast<CXXRecordDecl>(DC))
----------------
nit: will fails -> will fail


================
Comment at: clang/lib/Sema/SemaExpr.cpp:2588
+    if (const auto *CD = dyn_cast<CXXRecordDecl>(DC))
+      if (CD->isInvalidDecl())
+        return ExprError();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86765

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to