Rework the way we handle template instantiation for

implicitly-generated AST nodes. We previously built instantiated nodes
for each of these AST nodes, then passed them on to Sema, which was
not prepared to see already-type-checked nodes (see PR5755). In some
places, we had ugly workarounds to try to avoid re-type-checking
(e.g., in VarDecl initializer instantiation).

Now, we skip implicitly-generated nodes when performing instantiation,
preferring instead to build just the AST nodes that directly reflect
what was written in the source code. This has several advantages:

  - We don't need to instantiate anything that doesn't have a direct
    correlation to the source code, so we can have better location
    information.
  - Semantic analysis sees the same thing at template instantiation
    time that it would see for a non-template.
  - At least one ugly hack (VarDecl initializers) goes away.

Fixes PR5755.

llvm-svn: 91218

clang/include/clang/AST/ExprCXX.h

@@ -527,6 +527,7 @@ public:
   const_arg_iterator arg_begin() const { return Args; }
   const_arg_iterator arg_end() const { return Args + NumArgs; }
 
+  Expr **getArgs() const { return reinterpret_cast<Expr **>(Args); }
   unsigned getNumArgs() const { return NumArgs; }
 
   /// getArg - Return the specified argument.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -16,6 +16,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Lex/Preprocessor.h"
 
@@ -204,16 +205,6 @@ Decl *TemplateDeclInstantiator::VisitVarDecl(VarDecl *D) {
       = SemaRef.SubstExpr(D->getInit(), TemplateArgs);
     if (Init.isInvalid())
       Var->setInvalidDecl();
-    else if (!D->getType()->isDependentType() &&
-             !D->getInit()->isTypeDependent() &&
-             !D->getInit()->isValueDependent()) {
-      // If neither the declaration's type nor its initializer are dependent,
-      // we don't want to redo all the checking, especially since the
-      // initializer might have been wrapped by a CXXConstructExpr since we did
-      // it the first time.
-      Var->setType(D->getType());
-      Var->setInit(SemaRef.Context, Init.takeAs<Expr>());
-    }
     else if (ParenListExpr *PLE = dyn_cast<ParenListExpr>((Expr *)Init.get())) {
       // FIXME: We're faking all of the comma locations, which is suboptimal.
       // Do we even need these comma locations?
@@ -239,7 +230,47 @@ Decl *TemplateDeclInstantiator::VisitVarDecl(VarDecl *D) {
 
       // When Init is destroyed, it will destroy the instantiated ParenListExpr;
       // we've explicitly retained all of its subexpressions already.
-    } else
+    } else if (CXXConstructExpr *Construct
+                 = dyn_cast<CXXConstructExpr>((Expr *)Init.get())) {
+      // We build CXXConstructExpr nodes to capture the implicit
+      // construction of objects. Rip apart the CXXConstructExpr to
+      // pass its pieces down to the appropriate initialization
+      // function.
+      if (D->hasCXXDirectInitializer()) {
+        // FIXME: Poor source location information
+        SourceLocation FakeLParenLoc = 
+          SemaRef.PP.getLocForEndOfToken(D->getLocation());
+        SourceLocation FakeRParenLoc = FakeLParenLoc;
+        llvm::SmallVector<SourceLocation, 4> FakeCommaLocs;
+        if (Construct->getNumArgs() > 0) {
+          FakeRParenLoc 
+            = SemaRef.PP.getLocForEndOfToken(
+                  Construct->getArg(Construct->getNumArgs() - 1)->getLocEnd());
+
+          FakeCommaLocs.reserve(Construct->getNumArgs() - 1);
+          for (unsigned I = 0, N = Construct->getNumArgs() - 1; I != N; ++I) {
+            Expr *E = Construct->getArg(I)->Retain();
+            FakeCommaLocs.push_back(
+                                SemaRef.PP.getLocForEndOfToken(E->getLocEnd()));
+          }
+          Construct->getArg(Construct->getNumArgs() - 1)->Retain();
+        }
+
+        SemaRef.AddCXXDirectInitializerToDecl(Sema::DeclPtrTy::make(Var),
+                                              FakeLParenLoc,
+                               Sema::MultiExprArg(SemaRef,
+                                                  (void **)Construct->getArgs(),
+                                                  Construct->getNumArgs()),
+                                              FakeCommaLocs.data(),
+                                              FakeRParenLoc);
+                                              
+      } else if (Construct->getNumArgs() >= 1) {
+        SemaRef.AddInitializerToDecl(Sema::DeclPtrTy::make(Var), 
+                                  SemaRef.Owned(Construct->getArg(0)->Retain()),
+                                     false);
+      } else
+        SemaRef.ActOnUninitializedDecl(Sema::DeclPtrTy::make(Var), false);
+    } else 
       SemaRef.AddInitializerToDecl(Sema::DeclPtrTy::make(Var), move(Init),
                                    D->hasCXXDirectInitializer());
     SemaRef.PopExpressionEvaluationContext();

clang/lib/Sema/TreeTransform.h

@@ -993,19 +993,6 @@ public:
                                         move(LHS), move(RHS));
   }
 
-  /// \brief Build a new implicit cast expression.
-  ///
-  /// By default, builds a new implicit cast without any semantic analysis.
-  /// Subclasses may override this routine to provide different behavior.
-  OwningExprResult RebuildImplicitCastExpr(QualType T, CastExpr::CastKind Kind,
-                                           ExprArg SubExpr, bool isLvalue) {
-    ImplicitCastExpr *ICE
-      = new (getSema().Context) ImplicitCastExpr(T, Kind,
-                                                 (Expr *)SubExpr.release(),
-                                                 isLvalue);
-    return getSema().Owned(ICE);
-  }
-
   /// \brief Build a new C-style cast expression.
   ///
   /// By default, performs semantic analysis to build the new expression.
@@ -3779,29 +3766,39 @@ TreeTransform<Derived>::TransformConditionalOperator(ConditionalOperator *E) {
                                                  move(RHS));
 }
 
+/// \brief Given a cast expression, extract the subexpression of the
+/// cast, looking through intermediate AST nodes that were generated
+/// as part of type checking.
+static Expr *getCastSubExprAsWritten(CastExpr *E) {
+  Expr *SubExpr = 0;
+  do {
+    SubExpr = E->getSubExpr();
+
+    // Temporaries will be re-bound when rebuilding the original cast
+    // expression.
+    if (CXXBindTemporaryExpr *Binder = dyn_cast<CXXBindTemporaryExpr>(SubExpr))
+      SubExpr = Binder->getSubExpr();
+
+    // Conversions by constructor and conversion functions have a
+    // subexpression describing the call; strip it off.
+    if (E->getCastKind() == CastExpr::CK_ConstructorConversion)
+      SubExpr = cast<CXXConstructExpr>(SubExpr)->getArg(0);
+    else if (E->getCastKind() == CastExpr::CK_UserDefinedConversion)
+      SubExpr = cast<CXXMemberCallExpr>(SubExpr)->getImplicitObjectArgument();
+
+    // If the subexpression we're left with is an implicit cast, look
+    // through that, too.
+  } while ((E = dyn_cast<ImplicitCastExpr>(SubExpr)));
+
+  return SubExpr;
+}
+
 template<typename Derived>
 Sema::OwningExprResult
 TreeTransform<Derived>::TransformImplicitCastExpr(ImplicitCastExpr *E) {
-  TemporaryBase Rebase(*this, E->getLocStart(), DeclarationName());
-
-  // FIXME: Will we ever have type information here? It seems like we won't,
-  // so do we even need to transform the type?
-  QualType T = getDerived().TransformType(E->getType());
-  if (T.isNull())
-    return SemaRef.ExprError();
-
-  OwningExprResult SubExpr = getDerived().TransformExpr(E->getSubExpr());
-  if (SubExpr.isInvalid())
-    return SemaRef.ExprError();
-
-  if (!getDerived().AlwaysRebuild() &&
-      T == E->getType() &&
-      SubExpr.get() == E->getSubExpr())
-    return SemaRef.Owned(E->Retain());
-
-  return getDerived().RebuildImplicitCastExpr(T, E->getCastKind(),
-                                              move(SubExpr),
-                                              E->isLvalueCast());
+  // Implicit casts are eliminated during transformation, since they
+  // will be recomputed by semantic analysis after transformation.
+  return getDerived().TransformExpr(getCastSubExprAsWritten(E));
 }
 
 template<typename Derived>
@@ -3826,7 +3823,8 @@ TreeTransform<Derived>::TransformCStyleCastExpr(CStyleCastExpr *E) {
       return SemaRef.ExprError();
   }
 
-  OwningExprResult SubExpr = getDerived().TransformExpr(E->getSubExpr());
+  OwningExprResult SubExpr
+    = getDerived().TransformExpr(getCastSubExprAsWritten(E));
   if (SubExpr.isInvalid())
     return SemaRef.ExprError();
 
@@ -4182,7 +4180,8 @@ TreeTransform<Derived>::TransformCXXNamedCastExpr(CXXNamedCastExpr *E) {
       return SemaRef.ExprError();
   }
 
-  OwningExprResult SubExpr = getDerived().TransformExpr(E->getSubExpr());
+  OwningExprResult SubExpr
+    = getDerived().TransformExpr(getCastSubExprAsWritten(E));
   if (SubExpr.isInvalid())
     return SemaRef.ExprError();
 
@@ -4246,7 +4245,8 @@ TreeTransform<Derived>::TransformCXXFunctionalCastExpr(
       return SemaRef.ExprError();
   }
 
-  OwningExprResult SubExpr = getDerived().TransformExpr(E->getSubExpr());
+  OwningExprResult SubExpr
+    = getDerived().TransformExpr(getCastSubExprAsWritten(E));
   if (SubExpr.isInvalid())
     return SemaRef.ExprError();
 

clang/test/SemaTemplate/instantiate-expr-4.cpp

@@ -96,6 +96,18 @@ template struct Delete0<int*>;
 template struct Delete0<X*>;
 template struct Delete0<int>; // expected-note{{instantiation}}
 
+namespace PR5755 {
+  template <class T>
+  void Foo() {
+    char* p = 0;
+    delete[] p;
+  }
+  
+  void Test() {
+    Foo<int>();
+  }
+}
+
 // ---------------------------------------------------------------------
 // throw expressions
 // ---------------------------------------------------------------------