[NFC] Fixed an issue where diagnostics printed expressions in a roundabout way

[wchilders]

Summary

Previously expressions were being printed in diagnostics via an implicit conversion to an expression template argument -- which would then be printed via some code with a comment suggesting the code path was only for completeness and rarely/never used; this was both roundabout, and subtle. This patch provides a more direct overload, though the patch is still imperfect, as it does not solve the associated LangOpts issue.

Additionally, to prevent similar issues in the future, this patch makes conversions from expressions to template arguments explicit.

[wchilders]

Formatting fixes

[hans]

clang/lib/AST/TemplateBase.cpp

485

This code looks like a very special case, and the LangOptions comments suggests it's a bit hacky.
Because of that I'm not sure it makes sense to promote it to what looks like a general-purpose operator<< for Expr. What's the motivation for that?

clang/lib/Sema/SemaTemplateDeduction.cpp

463

nit: I guess TV is tor template value, but maybe TA would be a better name?

clang/include/clang/AST/ASTNodeTraverser.h

if (D->hasDefaultArgument())
             D->getDefaultArgStorage().getInheritedFrom(),
             D->defaultArgumentWasInherited() ? "inherited from" : "previous");
   }
 
   void VisitNonTypeTemplateParmDecl(const NonTypeTemplateParmDecl *D) {
     if (const auto *E = D->getPlaceholderTypeConstraint())
       Visit(E);
     if (D->hasDefaultArgument())
-      Visit(D->getDefaultArgument(), SourceRange(),
+      Visit(TemplateArgument(D->getDefaultArgument()), SourceRange(),
             D->getDefaultArgStorage().getInheritedFrom(),
             D->defaultArgumentWasInherited() ? "inherited from" : "previous");
   }
 
   void VisitTemplateTemplateParmDecl(const TemplateTemplateParmDecl *D) {
     dumpTemplateParameters(D->getTemplateParameters());
     if (D->hasDefaultArgument())
       dumpTemplateArgumentLoc(

clang/include/clang/AST/Expr.h

private:
 
   SourceLocation BeginLoc, EndLoc;
   unsigned NumExprs;
   friend TrailingObjects;
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 };
 
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
+                                      const Expr *E);
+
 } // end namespace clang
 
 #endif // LLVM_CLANG_AST_EXPR_H

clang/include/clang/AST/TemplateBase.h

else
       TemplateArg.NumExpansions = 0;
   }
 
   /// Construct a template argument that is an expression.
   ///
   /// This form of template argument only occurs in template argument
   /// lists used for dependent types and for expression; it will not
   /// occur in a non-dependent, canonical template argument list.
-  TemplateArgument(Expr *E) {
+  explicit TemplateArgument(Expr *E) {
     TypeOrValue.Kind = Expression;
     TypeOrValue.V = reinterpret_cast<uintptr_t>(E);
   }
 
   /// Construct a template argument that is a template argument pack.
   ///
   /// We assume that storage for the template arguments provided
   /// outlives the TemplateArgument itself.

clang/lib/AST/Expr.cpp

OMPIteratorExpr *OMPIteratorExpr::CreateEmpty(const ASTContext &Context,
                                               unsigned NumIterators) {
   void *Mem = Context.Allocate(
       totalSizeToAlloc<Decl *, Expr *, SourceLocation, OMPIteratorHelperData>(
           NumIterators, NumIterators * static_cast<int>(RangeExprOffset::Total),
           NumIterators * static_cast<int>(RangeLocOffset::Total), NumIterators),
       alignof(OMPIteratorExpr));
   return new (Mem) OMPIteratorExpr(EmptyShell(), NumIterators);
 }
+
+const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
+                                             const Expr *E) {
+  // FIXME: We're guessing at LangOptions!
+  SmallString<32> Str;
+  llvm::raw_svector_ostream OS(Str);
+  LangOptions LangOpts;
+  LangOpts.CPlusPlus = true;
+  PrintingPolicy Policy(LangOpts);
+  E->printPretty(OS, nullptr, Policy);
+  return DB << OS.str();
+}

clang/lib/AST/ItaniumMangle.cpp

void CXXNameMangler::mangleType(const ConstantMatrixType *T) {
   mangleIntegerLiteral(ASTCtx.getSizeType(), Columns);
   mangleType(T->getElementType());
 }
 
 void CXXNameMangler::mangleType(const DependentSizedMatrixType *T) {
   // U<Len>matrix_type<row expr><column expr><element type>
   StringRef VendorQualifier = "matrix_type";
   Out << "U" << VendorQualifier.size() << VendorQualifier;
-  mangleTemplateArg(T->getRowExpr());
-  mangleTemplateArg(T->getColumnExpr());
+  mangleTemplateArg(TemplateArgument(T->getRowExpr()));
+  mangleTemplateArg(TemplateArgument(T->getColumnExpr()));
   mangleType(T->getElementType());
 }
 
 void CXXNameMangler::mangleType(const DependentAddressSpaceType *T) {
   SplitQualType split = T->getPointeeType().split();
   mangleQualifiers(split.Quals, T);
   mangleType(QualType(split.Ty, 0));
 }

clang/lib/AST/JSONNodeDumper.cpp

void JSONNodeDumper::VisitNonTypeTemplateParmDecl(
   VisitNamedDecl(D);
   JOS.attribute("type", createQualType(D->getType()));
   JOS.attribute("depth", D->getDepth());
   JOS.attribute("index", D->getIndex());
   attributeOnlyIfTrue("isParameterPack", D->isParameterPack());
 
   if (D->hasDefaultArgument())
     JOS.attributeObject("defaultArg", [=] {
-      Visit(D->getDefaultArgument(), SourceRange(),
+      Visit(TemplateArgument(D->getDefaultArgument()), SourceRange(),
             D->getDefaultArgStorage().getInheritedFrom(),
             D->defaultArgumentWasInherited() ? "inherited from" : "previous");
     });
 }
 
 void JSONNodeDumper::VisitTemplateTemplateParmDecl(
     const TemplateTemplateParmDecl *D) {
   VisitNamedDecl(D);

clang/lib/AST/TemplateBase.cpp

 TemplateArgument TemplateArgument::getPackExpansionPattern() const {
   assert(isPackExpansion());
 
   switch (getKind()) {
   case Type:
     return getAsType()->castAs<PackExpansionType>()->getPattern();
 
   case Expression:
-    return cast<PackExpansionExpr>(getAsExpr())->getPattern();
+    return TemplateArgument(cast<PackExpansionExpr>(getAsExpr())->getPattern());
 
   case TemplateExpansion:
     return TemplateArgument(getAsTemplateOrTemplatePattern());
 
   case Declaration:
   case Integral:
   case Pack:
   case Null:
static const T &DiagTemplateArg(const T &DB, const TemplateArgument &Arg) {
 
   case TemplateArgument::Template:
     return DB << Arg.getAsTemplate();
 
   case TemplateArgument::TemplateExpansion:
     return DB << Arg.getAsTemplateOrTemplatePattern() << "...";
 
   case TemplateArgument::Expression: {
-    // This shouldn't actually ever happen, so it's okay that we're
-    // regurgitating an expression here.
-    // FIXME: We're guessing at LangOptions!
-    SmallString<32> Str;
-    llvm::raw_svector_ostream OS(Str);
-    LangOptions LangOpts;
-    LangOpts.CPlusPlus = true;
-    PrintingPolicy Policy(LangOpts);
-    Arg.getAsExpr()->printPretty(OS, nullptr, Policy);
-    return DB << OS.str();
+    return DB << Arg.getAsExpr();
   }
 
   case TemplateArgument::Pack: {
     // FIXME: We're guessing at LangOptions!
     SmallString<32> Str;
     llvm::raw_svector_ostream OS(Str);
     LangOptions LangOpts;
     LangOpts.CPlusPlus = true;

clang/lib/Sema/SemaTemplateDeduction.cpp

static Sema::TemplateDeductionResult DeduceNullPtrTemplateArgument(
     const NonTypeTemplateParmDecl *NTTP, QualType NullPtrType,
     TemplateDeductionInfo &Info,
     SmallVectorImpl<DeducedTemplateArgument> &Deduced) {
   Expr *Value =
       S.ImpCastExprToType(new (S.Context) CXXNullPtrLiteralExpr(
                               S.Context.NullPtrTy, NTTP->getLocation()),
                           NullPtrType, CK_NullToPointer)
           .get();
+  TemplateArgument TV(Value);
   return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
-                                       DeducedTemplateArgument(Value),
+                                       DeducedTemplateArgument(TV),
                                        Value->getType(), Info, Deduced);
 }
 
 /// Deduce the value of the given non-type template parameter
 /// from the given type- or value-dependent expression.
 ///
 /// \returns true if deduction succeeded, false otherwise.
 static Sema::TemplateDeductionResult DeduceNonTypeTemplateArgument(
     Sema &S, TemplateParameterList *TemplateParams,
     const NonTypeTemplateParmDecl *NTTP, Expr *Value, TemplateDeductionInfo &Info,
     SmallVectorImpl<DeducedTemplateArgument> &Deduced) {
+  TemplateArgument TV(Value);
   return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
-                                       DeducedTemplateArgument(Value),
+                                       DeducedTemplateArgument(TV),
                                        Value->getType(), Info, Deduced);
 }
 
 /// Deduce the value of the given non-type template parameter
 /// from the given declaration.
 ///
 /// \returns true if deduction succeeded, false otherwise.
 static Sema::TemplateDeductionResult DeduceNonTypeTemplateArgument(

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

static void instantiateDependentAlignedAttr(
     const AlignedAttr *Aligned, Decl *New) {
   if (!Aligned->isPackExpansion()) {
     instantiateDependentAlignedAttr(S, TemplateArgs, Aligned, New, false);
     return;
   }
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   if (Aligned->isAlignmentExpr())
-    S.collectUnexpandedParameterPacks(Aligned->getAlignmentExpr(),
-                                      Unexpanded);
+    S.collectUnexpandedParameterPacks(
+        TemplateArgument(Aligned->getAlignmentExpr()), Unexpanded);
   else
     S.collectUnexpandedParameterPacks(Aligned->getAlignmentType()->getTypeLoc(),
                                       Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether we can expand this attribute pack yet.
   bool Expand = true, RetainExpansion = false;
   Optional<unsigned> NumExpansions;
for (const auto *Init : Tmpl->inits()) {
 
     SourceLocation EllipsisLoc;
 
     if (Init->isPackExpansion()) {
       // This is a pack expansion. We should expand it now.
       TypeLoc BaseTL = Init->getTypeSourceInfo()->getTypeLoc();
       SmallVector<UnexpandedParameterPack, 4> Unexpanded;
       collectUnexpandedParameterPacks(BaseTL, Unexpanded);
-      collectUnexpandedParameterPacks(Init->getInit(), Unexpanded);
+      collectUnexpandedParameterPacks(TemplateArgument(Init->getInit()),
+                                      Unexpanded);
       bool ShouldExpand = false;
       bool RetainExpansion = false;
       Optional<unsigned> NumExpansions;
       if (CheckParameterPacksForExpansion(Init->getEllipsisLoc(),
                                           BaseTL.getSourceRange(),
                                           Unexpanded,
                                           TemplateArgs, ShouldExpand,
                                           RetainExpansion,

clang/lib/Sema/SemaTemplateVariadic.cpp

Sema::getTemplateArgumentPackExpansionPattern(
   }
 
   case TemplateArgument::Expression: {
     PackExpansionExpr *Expansion
       = cast<PackExpansionExpr>(Argument.getAsExpr());
     Expr *Pattern = Expansion->getPattern();
     Ellipsis = Expansion->getEllipsisLoc();
     NumExpansions = Expansion->getNumExpansions();
-    return TemplateArgumentLoc(Pattern, Pattern);
+    return TemplateArgumentLoc(TemplateArgument(Pattern), Pattern);
   }
 
   case TemplateArgument::TemplateExpansion:
     Ellipsis = OrigLoc.getTemplateEllipsisLoc();
     NumExpansions = Argument.getNumTemplateExpansions();
     return TemplateArgumentLoc(Context, Argument.getPackExpansionPattern(),
                                OrigLoc.getTemplateQualifierLoc(),
                                OrigLoc.getTemplateNameLoc());

clang/lib/Sema/TreeTransform.h

TemplateArgumentLoc RebuildPackExpansion(TemplateArgumentLoc Pattern,
     switch (Pattern.getArgument().getKind()) {
     case TemplateArgument::Expression: {
       ExprResult Result
         = getSema().CheckPackExpansion(Pattern.getSourceExpression(),
                                        EllipsisLoc, NumExpansions);
       if (Result.isInvalid())
         return TemplateArgumentLoc();
 
-      return TemplateArgumentLoc(Result.get(), Result.get());
+      return TemplateArgumentLoc(TemplateArgument(Result.get()), Result.get());
     }
 
     case TemplateArgument::Template:
       return TemplateArgumentLoc(
           SemaRef.Context,
           TemplateArgument(Pattern.getArgument().getAsTemplate(),
                            NumExpansions),
           Pattern.getTemplateQualifierLoc(), Pattern.getTemplateNameLoc(),
if (IsCall && getDerived().DropCallArgument(Inputs[I])) {
 
       break;
     }
 
     if (PackExpansionExpr *Expansion = dyn_cast<PackExpansionExpr>(Inputs[I])) {
       Expr *Pattern = Expansion->getPattern();
 
       SmallVector<UnexpandedParameterPack, 2> Unexpanded;
-      getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded);
+      getSema().collectUnexpandedParameterPacks(TemplateArgument(Pattern),
+                                                Unexpanded);
       assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
       // Determine whether the set of unexpanded parameter packs can and should
       // be expanded.
       bool Expand = true;
       bool RetainExpansion = false;
       Optional<unsigned> OrigNumExpansions = Expansion->getNumExpansions();
       Optional<unsigned> NumExpansions = OrigNumExpansions;
for (LambdaExpr::capture_iterator C = E->capture_begin(),
     };
 
     // If this is an init-capture pack, consider expanding the pack now.
     if (OldVD->isParameterPack()) {
       PackExpansionTypeLoc ExpansionTL = OldVD->getTypeSourceInfo()
                                              ->getTypeLoc()
                                              .castAs<PackExpansionTypeLoc>();
       SmallVector<UnexpandedParameterPack, 2> Unexpanded;
-      SemaRef.collectUnexpandedParameterPacks(OldVD->getInit(), Unexpanded);
+      SemaRef.collectUnexpandedParameterPacks(
+          TemplateArgument(OldVD->getInit()), Unexpanded);
 
       // Determine whether the set of unexpanded parameter packs can and should
       // be expanded.
       bool Expand = true;
       bool RetainExpansion = false;
       Optional<unsigned> OrigNumExpansions =
           ExpansionTL.getTypePtr()->getNumExpansions();
       Optional<unsigned> NumExpansions = OrigNumExpansions;
if (ShouldExpand) {
       } else {
         auto *VD = cast<ValueDecl>(Pack);
         ExprResult DRE = getSema().BuildDeclRefExpr(
             VD, VD->getType().getNonLValueExprType(getSema().Context),
             VD->getType()->isReferenceType() ? VK_LValue : VK_RValue,
             E->getPackLoc());
         if (DRE.isInvalid())
           return ExprError();
-        ArgStorage = new (getSema().Context) PackExpansionExpr(
-            getSema().Context.DependentTy, DRE.get(), E->getPackLoc(), None);
+        ArgStorage = TemplateArgument(new (getSema().Context) PackExpansionExpr(
+            getSema().Context.DependentTy, DRE.get(), E->getPackLoc(), None));
       }
       PackArgs = ArgStorage;
     }
   }
 
   // If we're not expanding the pack, just transform the decl.
   if (!PackArgs.size()) {
     auto *Pack = cast_or_null<NamedDecl>(
if (Expr *OldCallee = E->getCallee()) {
     if (CalleeResult.isInvalid())
       return ExprError();
     Callee = cast<UnresolvedLookupExpr>(CalleeResult.get());
   }
 
   Expr *Pattern = E->getPattern();
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
-  getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(TemplateArgument(Pattern),
+                                            Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can and should
   // be expanded.
   bool Expand = true;
   bool RetainExpansion = false;
   Optional<unsigned> OrigNumExpansions = E->getNumExpansions(),
                      NumExpansions = OrigNumExpansions;
TreeTransform<Derived>::TransformObjCDictionaryLiteral(
   SmallVector<ObjCDictionaryElement, 8> Elements;
   bool ArgChanged = false;
   for (unsigned I = 0, N = E->getNumElements(); I != N; ++I) {
     ObjCDictionaryElement OrigElement = E->getKeyValueElement(I);
 
     if (OrigElement.isPackExpansion()) {
       // This key/value element is a pack expansion.
       SmallVector<UnexpandedParameterPack, 2> Unexpanded;
-      getSema().collectUnexpandedParameterPacks(OrigElement.Key, Unexpanded);
-      getSema().collectUnexpandedParameterPacks(OrigElement.Value, Unexpanded);
+      getSema().collectUnexpandedParameterPacks(
+          TemplateArgument(OrigElement.Key), Unexpanded);
+      getSema().collectUnexpandedParameterPacks(
+          TemplateArgument(OrigElement.Value), Unexpanded);
       assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
       // Determine whether the set of unexpanded parameter packs can
       // and should be expanded.
       bool Expand = true;
       bool RetainExpansion = false;
       Optional<unsigned> OrigNumExpansions = OrigElement.NumExpansions;
       Optional<unsigned> NumExpansions = OrigNumExpansions;