[PATCH] Sema: Instantiate local class and their members appropriately

[Richard Smith]

  Thanks, it'll be great to finally have this bug closed.


================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:2503-2506
@@ -2496,2 +2502,6 @@
                       
-          InstantiateFunctionDefinition(PointOfInstantiation, Function);
+          if (TSK == TSK_ImplicitInstantiation)
+            PendingLocalImplicitInstantiations.push_back(
+                std::make_pair(Function, PointOfInstantiation));
+          else
+            InstantiateFunctionDefinition(PointOfInstantiation, Function);
----------------
Might be clearer to move this into the `else` below? The `Pattern->isDefined()` check and associated comment don't apply here.

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:671-672
@@ -659,4 +670,4 @@
 
-  if (D->getDeclContext()->isFunctionOrMethod())
+  if (isDeclWithinFunction(D))
     SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, Enum);
 
----------------
I would expect we need to do this iff we call `InstantiateEnumDefinition`. In all other cases, it'll be done by the caller; we need to push `D` into the `CurrentInstantiationScope` if (and only if) we might instantiate something ourselves that references the enumeration type.

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:726-727
@@ -725,4 +735,3 @@
 
-      if (Pattern->getDeclContext()->isFunctionOrMethod() &&
-          !Enum->isScoped()) {
         // If the enumeration is within a function or method, record the enum
----------------
Is this change necessary? If the enumerator is within a local class, we should (and should be able to) find it by name lookup in the instantiation of the class, rather than through the current instantiation scope.

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:1140-1151
@@ -1130,1 +1139,14 @@
+
+  if (CXXRecordDecl *Def = D->getDefinition()) {
+    if (Def->isLocalClass()) {
+      if (SemaRef.InstantiateClass(D->getLocation(), Record, Def, TemplateArgs,
+                                   TSK_ImplicitInstantiation,
+                                   /*Complain=*/true)) {
+        llvm_unreachable("InstantiateClass shouldn't fail here!");
+      } else {
+        SemaRef.InstantiateClassMembers(D->getLocation(), Record, TemplateArgs,
+                                        TSK_ImplicitInstantiation);
+      }
+    }
+  }
   return Record;
----------------
I think we should only do this if `D` is the definition. If we have:

  template<typename T> void f() {
    struct X; // #1
    struct X {}; // #2
  }

we should instantiate X when we get to #2, not when we get to #1. This is probably even detectable with something like:

  template<typename T> void f() {
    struct X;
    int k = X().n;
    struct X { int n; };
  }

This is ill-formed, but Clang (and GCC, as it happens) accepts-invalid on it. (I expect we'll still accept-valid even with your patch, because we'll trigger the instantiation of X() on the second line rather than the first/third, but it's a step in the right direction.)

================
Comment at: test/CXX/temp/temp.spec/temp.inst/p1.cpp:36-38
@@ -35,5 +35,5 @@
 
   // The behavior for enums defined within function templates is not clearly
   // specified by the standard. We follow the rules for enums defined within
   // class templates.
   template<typename T>
----------------
Please update this comment to refer to core issue 1484, which explains the rules here.

================
Comment at: test/SemaCXX/local-classes.cpp:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
 // expected-no-diagnostics
----------------
We have a whole bunch of examples on the PRs for this bug; please include a few more of them here.


http://llvm-reviews.chandlerc.com/D2236