[PATCH] Handle use of default member initializers before end of outermost class

[Reid Kleckner]

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6189
@@ +6188,3 @@
+def err_in_class_initializer_not_yet_parsed : Error<
+  "cannot default initialize non-static data member %0 before the end of the "
+  "outermost containing class %1">;
----------------
rsmith wrote:
> "default initialize" is the wrong term here; it means something else.
> 
> The existing diagnostic was specifically targeted at http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1397, and its wording was intended to explain what the relevant rule is. I'm not sure how best to update the diagnostic text, but this wording needs more work.
The other diagnostic was much more specific, but it fired too late, so we ended up with both diagnostics.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8420-8435
@@ -8419,18 +8420,2 @@
         ExceptSpec.CalledExpr(E);
-      else if (!F->isInvalidDecl())
-        // DR1351:
-        //   If the brace-or-equal-initializer of a non-static data member
-        //   invokes a defaulted default constructor of its class or of an
-        //   enclosing class in a potentially evaluated subexpression, the
-        //   program is ill-formed.
-        //
-        // This resolution is unworkable: the exception specification of the
-        // default constructor can be needed in an unevaluated context, in
-        // particular, in the operand of a noexcept-expression, and we can be
-        // unable to compute an exception specification for an enclosed class.
-        //
-        // We do not allow an in-class initializer to require the evaluation
-        // of the exception specification for any in-class initializer whose
-        // definition is not lexically complete.
-        Diag(Loc, diag::err_in_class_initializer_references_def_ctor) << MD;
     } else if (const RecordType *RecordTy
----------------
rsmith wrote:
> You can trigger the computation of the exception specification for a defaulted constructor without causing it to be defined (or at least, in principle; it looks like Clang doesn't get this right). If you do so from a default initializer, we're required to reject the program under DR1397, and we shouldn't just ignore the fact that there was a default initializer.
I wonder if r218466 is related: "Move calls to ResolveExceptionSpec into DefineImplicit*".

================
Comment at: lib/Sema/SemaDeclCXX.cpp:10979-10981
@@ +10978,5 @@
+  if (isTemplateInstantiation(ParentRD->getTemplateSpecializationKind())) {
+    auto *InstantiatedClass = cast<ClassTemplateSpecializationDecl>(ParentRD);
+    CXXRecordDecl *ClassPattern =
+        InstantiatedClass->getSpecializedTemplate()->getTemplatedDecl();
+    DeclContext::lookup_result Lookup =
----------------
rsmith wrote:
> This isn't right. You need to handle two cases:
> 
> 1) The class is an instantiation of a class template or partial specialization; cast to `ClassTemplateSpecializationDecl` and call `getInstantiatedFrom()`.
> 2) The class is a member of a class template (recursively), and is not a `ClassTemplateSpecializationDecl`. In this case, call `getInstantiatedFromMemberClass` to find the pattern.
> 
> In the first case, you may also need to walk from a member template or member partial specailization back to its pattern (and be careful to stop when you hit a member specialization). See `hasVisibleDefinition` in SemaType.cpp and `FunctionDecl::getTemplateInstantiationPattern` for examples of where we already do this.
> 
> It seems like a good idea to move this into a new `CXXRecordDecl::getTemplateInstantiationPattern`.
I think this is addressed now. I can't test the explicit and partial specialization cases because I can't seem to declare such specializations before leaving class scope, which hides this codepath.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:10992-10995
@@ +10991,6 @@
+  RecordDecl *OutermostClass = ParentRD;
+  for (DeclContext *DC = OutermostClass; !DC->isTranslationUnit();
+       DC = DC->getParent()) {
+    if (auto *RD = dyn_cast<RecordDecl>(DC))
+      OutermostClass = RD;
+  }
----------------
rsmith wrote:
> You should walk over lexically-enclosing contexts, and stop when you hit the first context that's not a `CXXRecordDecl`. For a local class, surrounding classes don't matter, and for an out-of-line definition of a nested class, its containing class doesn't matter.
Should be fixed with DeclContext::getOuterLexicalRecordContext().

================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:2033-2040
@@ -2032,10 +2032,10 @@
 
-      ActOnStartCXXInClassMemberInitializer();
-      ExprResult NewInit = SubstInitializer(OldInit, TemplateArgs,
-                                            /*CXXDirectInit=*/false);
-      Expr *Init = NewInit.get();
-      assert((!Init || !isa<ParenListExpr>(Init)) &&
-             "call-style init in class");
-      ActOnFinishCXXInClassMemberInitializer(NewField,
-        Init ? Init->getLocStart() : SourceLocation(), Init);
+      // If we haven't parsed the initializer yet, defer instantiation until
+      // later.
+      // FIXME: If we can tolerate defering some initializers, it shouldn't be
+      // hard to defer *all* field instantiation until later as described above.
+      if (OldInit) {
+        InstantiateField(Instantiation->getLocation(), NewField, OldField,
+                         TemplateArgs);
+      }
     }
----------------
rsmith wrote:
> Do we still set the 'this field has an initializer' flag somewhere?
Yes, TemplateDeclInstantiator::VisitFieldDecl() copies it from the pattern to the instantiation.

================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:2199
@@ +2198,3 @@
+bool
+Sema::InstantiateField(SourceLocation PointOfInstantiation,
+                       FieldDecl *Instantiation, FieldDecl *Pattern,
----------------
rsmith wrote:
> I'd prefer for this function to have a name related to in-class initializers, since that's the specific thing it's dealing with.
Feel free to pick one if you like it:
  InstantiateFieldInitializer
  InstantiateDefaultInitializer
  InstantiateInClassInitializer
I'm going with InstantiateInClassInitializer for now, since it's grep-able in relation to InClassInitializer.

================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:2228-2235
@@ +2227,10 @@
+  // Instantiate the initializer.
+  ActOnStartCXXInClassMemberInitializer();
+  ExprResult NewInit = SubstInitializer(OldInit, TemplateArgs,
+                                        /*CXXDirectInit=*/false);
+  Expr *Init = NewInit.get();
+  assert((!Init || !isa<ParenListExpr>(Init)) && "call-style init in class");
+  ActOnFinishCXXInClassMemberInitializer(
+      Instantiation, Init ? Init->getLocStart() : SourceLocation(), Init);
+  return Init == nullptr;
+}
----------------
rsmith wrote:
> You should create an `InstantiatingTemplate` surrounding this for proper handling of instantiation backtraces. (Note that you're currently not using `PointOfInstantiation`.)
> 
> You also need a `ContextRAII` and `LocalInstantiationScope` here so that (for instance) lambdas within the initializer are created in the right `DeclContext`, and we perform proper access checking, and so on.
> 
> You should also `EnterExpressionEvaluationContext` to ensure we're in a `PotentiallyEvaluated` context (the instantiation could be triggered by an aggregate initialization in an unevaluated operand).
Nice! This fixed everything and allowed me to completely defer instantiation of NSDMIs until we form CXXDefaultInitExprs.

http://reviews.llvm.org/D5690