[PATCH] D101641: [Sema] Preserve invalid CXXCtorInitializers using RecoveryExpr in initializer

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:4475
   //   name that denotes that base class type.
-  bool Dependent = BaseType->isDependentType() || Init->isTypeDependent();
+  bool Dependent = CurContext->isDependentContext() &&
+                   (BaseType->isDependentType() || Init->isTypeDependent());
----------------
sammccall wrote:
> hokein wrote:
> > it is unclear to me why we need a `isDependentContext()` here? looks like this would special-case RecoveryExpr, the following dependent code-path would not be executed for RecoveryExpr case? 
> Right, if the init expr is type-dependent because it contains errors, but we're not actually in a template, we don't want to take the dependent path.
> 
> The reason is that the dependent path produces an "as-written" form of the CXXCtorInitializer, which has weaker invariants, because we know we're not going to examine it much until template instantiation time.
> e.g. an initializer that is actually delegating is stored as a base initializer instead. (Thus avoiding the impossible requirement to check whether two dependent types are the same).
> Later on in Sema::SetCtorInitializers, we check `Constructor->isDependentContext()` to decide whether to propagate the "as-written" initializers without checking, but of course this is false. So then we go and shuffle the initializers into the correct order, build default initializers for the uninitialized members etc, and the delegating initializer is just... lost in the shuffle. We think it's a base init, so it's put in a map of type->init, and then it never gets consumed from there because the type is not actually a base class.
> 
> I'll try to add a comment that hints at this, without getting into the concrete details...
I see, fair enough, thanks for the classifications. 


================
Comment at: clang/test/AST/ast-dump-recovery.cpp:305
+    S s;
+    MemberInit() : x(invalid), y(invalid, invalid), s(1,2) {}
+    // CHECK:      CXXConstructorDecl {{.*}} MemberInit 'void ()'
----------------
For completeness, I would add case 'x(undefine())', even though it already works without this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101641