From 419563768ef4929a622d7c2b066856e82901bb91 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 14 Jan 2013 22:39:08 +0000 Subject: [PATCH] Refactor to call ActOnFinishFullExpr on every full expression. Teach ActOnFinishFullExpr that some of its checks only apply to discarded-value expressions. This adds missing checks for unexpanded variadic template parameter packs to a handful of constructs. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@172485 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Sema/Sema.h | 11 +++- lib/Parse/ParseObjc.cpp | 2 +- lib/Parse/ParseStmt.cpp | 10 +-- lib/Sema/SemaDecl.cpp | 22 +++++-- lib/Sema/SemaDeclCXX.cpp | 34 ++++------ lib/Sema/SemaExprCXX.cpp | 21 +++--- lib/Sema/SemaStmt.cpp | 66 ++++++++++++------- lib/Sema/TreeTransform.h | 4 +- test/CXX/temp/temp.decls/temp.variadic/p5.cpp | 9 +++ test/CXX/temp/temp.decls/temp.variadic/p5.mm | 9 +++ 10 files changed, 121 insertions(+), 67 deletions(-) create mode 100644 test/CXX/temp/temp.decls/temp.variadic/p5.mm diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index cd43d7cdfb..591ced11d5 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -2554,8 +2554,14 @@ public: FullExprArg MakeFullExpr(Expr *Arg, SourceLocation CC) { return FullExprArg(ActOnFinishFullExpr(Arg, CC).release()); } + FullExprArg MakeFullDiscardedValueExpr(Expr *Arg) { + ExprResult FE = + ActOnFinishFullExpr(Arg, Arg ? Arg->getExprLoc() : SourceLocation(), + /*DiscardedValue*/ true); + return FullExprArg(FE.release()); + } - StmtResult ActOnExprStmt(FullExprArg Expr); + StmtResult ActOnExprStmt(ExprResult Arg); StmtResult ActOnNullStmt(SourceLocation SemiLoc, bool HasLeadingEmptyMacro = false); @@ -3958,7 +3964,8 @@ public: return ActOnFinishFullExpr(Expr, Expr ? Expr->getExprLoc() : SourceLocation()); } - ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC); + ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC, + bool DiscardedValue = false); StmtResult ActOnFinishFullStmt(Stmt *Stmt); // Marks SS invalid if it represents an incomplete type. diff --git a/lib/Parse/ParseObjc.cpp b/lib/Parse/ParseObjc.cpp index f463902dda..ae1e19cc6d 100644 --- a/lib/Parse/ParseObjc.cpp +++ b/lib/Parse/ParseObjc.cpp @@ -2036,7 +2036,7 @@ StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc) { // Otherwise, eat the semicolon. ExpectAndConsumeSemi(diag::err_expected_semi_after_expr); - return Actions.ActOnExprStmt(Actions.MakeFullExpr(Res.take())); + return Actions.ActOnExprStmt(Res); } ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) { diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp index be77991b37..87c1d46f6c 100644 --- a/lib/Parse/ParseStmt.cpp +++ b/lib/Parse/ParseStmt.cpp @@ -335,7 +335,7 @@ StmtResult Parser::ParseExprStatement() { // Otherwise, eat the semicolon. ExpectAndConsumeSemi(diag::err_expected_semi_after_expr); - return Actions.ActOnExprStmt(Actions.MakeFullExpr(Expr.get())); + return Actions.ActOnExprStmt(Expr); } StmtResult Parser::ParseSEHTryBlock() { @@ -856,7 +856,7 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) { // Eat the semicolon at the end of stmt and convert the expr into a // statement. ExpectAndConsumeSemi(diag::err_expected_semi_after_expr); - R = Actions.ActOnExprStmt(Actions.MakeFullExpr(Res.get())); + R = Actions.ActOnExprStmt(Res); } } @@ -1437,7 +1437,7 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { if (ForEach) FirstPart = Actions.ActOnForEachLValueExpr(Value.get()); else - FirstPart = Actions.ActOnExprStmt(Actions.MakeFullExpr(Value.get())); + FirstPart = Actions.ActOnExprStmt(Value); } if (Tok.is(tok::semi)) { @@ -1505,7 +1505,9 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { // Parse the third part of the for specifier. if (Tok.isNot(tok::r_paren)) { // for (...;...;) ExprResult Third = ParseExpression(); - ThirdPart = Actions.MakeFullExpr(Third.take()); + // FIXME: The C++11 standard doesn't actually say that this is a + // discarded-value expression, but it clearly should be. + ThirdPart = Actions.MakeFullDiscardedValueExpr(Third.take()); } } // Match the ')'. diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index 7dc413e499..537e70bfbe 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -6873,9 +6873,6 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, if (!VDecl->isInvalidDecl() && (DclT != SavT)) VDecl->setType(DclT); - // Check any implicit conversions within the expression. - CheckImplicitConversions(Init, VDecl->getLocation()); - if (!VDecl->isInvalidDecl()) { checkUnsafeAssigns(VDecl->getLocation(), VDecl->getType(), Init); @@ -6898,7 +6895,24 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, } } - Init = MaybeCreateExprWithCleanups(Init); + // The initialization is usually a full-expression. + // + // FIXME: If this is a braced initialization of an aggregate, it is not + // an expression, and each individual field initializer is a separate + // full-expression. For instance, in: + // + // struct Temp { ~Temp(); }; + // struct S { S(Temp); }; + // struct T { S a, b; } t = { Temp(), Temp() } + // + // we should destroy the first Temp before constructing the second. + ExprResult Result = ActOnFinishFullExpr(Init, VDecl->getLocation()); + if (Result.isInvalid()) { + VDecl->setInvalidDecl(); + return; + } + Init = Result.take(); + // Attach the initializer to the decl. VDecl->setInit(Init); diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index b8926a9ca7..5650781893 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -1949,14 +1949,12 @@ Sema::ActOnCXXInClassMemberInitializer(Decl *D, SourceLocation InitLoc, FD->setInvalidDecl(); return; } - - CheckImplicitConversions(Init.get(), InitLoc); } - // C++0x [class.base.init]p7: + // C++11 [class.base.init]p7: // The initialization of each base and member constitutes a // full-expression. - Init = MaybeCreateExprWithCleanups(Init); + Init = ActOnFinishFullExpr(Init.take(), InitLoc); if (Init.isInvalid()) { FD->setInvalidDecl(); return; @@ -2371,13 +2369,10 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr *Init, if (MemberInit.isInvalid()) return true; - CheckImplicitConversions(MemberInit.get(), - InitRange.getBegin()); - - // C++0x [class.base.init]p7: + // C++11 [class.base.init]p7: // The initialization of each base and member constitutes a // full-expression. - MemberInit = MaybeCreateExprWithCleanups(MemberInit); + MemberInit = ActOnFinishFullExpr(MemberInit.get(), InitRange.getBegin()); if (MemberInit.isInvalid()) return true; @@ -2432,12 +2427,11 @@ Sema::BuildDelegatingInitializer(TypeSourceInfo *TInfo, Expr *Init, assert(cast(DelegationInit.get())->getConstructor() && "Delegating constructor with no target?"); - CheckImplicitConversions(DelegationInit.get(), InitRange.getBegin()); - - // C++0x [class.base.init]p7: + // C++11 [class.base.init]p7: // The initialization of each base and member constitutes a // full-expression. - DelegationInit = MaybeCreateExprWithCleanups(DelegationInit); + DelegationInit = ActOnFinishFullExpr(DelegationInit.get(), + InitRange.getBegin()); if (DelegationInit.isInvalid()) return true; @@ -2566,12 +2560,10 @@ Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo, if (BaseInit.isInvalid()) return true; - CheckImplicitConversions(BaseInit.get(), InitRange.getBegin()); - - // C++0x [class.base.init]p7: - // The initialization of each base and member constitutes a + // C++11 [class.base.init]p7: + // The initialization of each base and member constitutes a // full-expression. - BaseInit = MaybeCreateExprWithCleanups(BaseInit); + BaseInit = ActOnFinishFullExpr(BaseInit.get(), InitRange.getBegin()); if (BaseInit.isInvalid()) return true; @@ -8097,7 +8089,7 @@ buildSingleCopyAssignRecursively(Sema &S, SourceLocation Loc, QualType T, // Convert to an expression-statement, and clean up any produced // temporaries. - return S.ActOnExprStmt(S.MakeFullExpr(Call.take(), Loc)); + return S.ActOnExprStmt(Call); } // - if the subobject is of scalar type, the built-in assignment @@ -8107,7 +8099,7 @@ buildSingleCopyAssignRecursively(Sema &S, SourceLocation Loc, QualType T, ExprResult Assignment = S.CreateBuiltinBinOp(Loc, BO_Assign, To, From); if (Assignment.isInvalid()) return StmtError(); - return S.ActOnExprStmt(S.MakeFullExpr(Assignment.take(), Loc)); + return S.ActOnExprStmt(Assignment); } // - if the subobject is an array, each element is assigned, in the @@ -8184,7 +8176,7 @@ buildSingleCopyAssignRecursively(Sema &S, SourceLocation Loc, QualType T, // Construct the loop that copies all elements of this array. return S.ActOnForStmt(Loc, Loc, InitStmt, S.MakeFullExpr(Comparison), - 0, S.MakeFullExpr(Increment), + 0, S.MakeFullDiscardedValueExpr(Increment), Loc, Copy.take()); } diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp index f4f2c1c23a..05b5665ee9 100644 --- a/lib/Sema/SemaExprCXX.cpp +++ b/lib/Sema/SemaExprCXX.cpp @@ -5467,7 +5467,8 @@ ExprResult Sema::IgnoredValueConversions(Expr *E) { return Owned(E); } -ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC) { +ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC, + bool DiscardedValue) { ExprResult FullExpr = Owned(FE); if (!FullExpr.get()) @@ -5477,21 +5478,23 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC) { return ExprError(); // Top-level message sends default to 'id' when we're in a debugger. - if (getLangOpts().DebuggerCastResultToId && + if (DiscardedValue && getLangOpts().DebuggerCastResultToId && FullExpr.get()->getType() == Context.UnknownAnyTy && isa(FullExpr.get())) { FullExpr = forceUnknownAnyToType(FullExpr.take(), Context.getObjCIdType()); if (FullExpr.isInvalid()) return ExprError(); } - - FullExpr = CheckPlaceholderExpr(FullExpr.take()); - if (FullExpr.isInvalid()) - return ExprError(); - FullExpr = IgnoredValueConversions(FullExpr.take()); - if (FullExpr.isInvalid()) - return ExprError(); + if (DiscardedValue) { + FullExpr = CheckPlaceholderExpr(FullExpr.take()); + if (FullExpr.isInvalid()) + return ExprError(); + + FullExpr = IgnoredValueConversions(FullExpr.take()); + if (FullExpr.isInvalid()) + return ExprError(); + } CheckImplicitConversions(FullExpr.get(), CC); return MaybeCreateExprWithCleanups(FullExpr); diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 9d8c04a641..09c418072d 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -36,9 +36,13 @@ using namespace clang; using namespace sema; -StmtResult Sema::ActOnExprStmt(FullExprArg expr) { - Expr *E = expr.get(); - if (!E) // FIXME: FullExprArg has no error state? +StmtResult Sema::ActOnExprStmt(ExprResult FE) { + if (FE.isInvalid()) + return StmtError(); + + FE = ActOnFinishFullExpr(FE.get(), FE.get()->getExprLoc(), + /*DiscardedValue*/ true); + if (FE.isInvalid()) return StmtError(); // C99 6.8.3p2: The expression in an expression statement is evaluated as a @@ -46,7 +50,7 @@ StmtResult Sema::ActOnExprStmt(FullExprArg expr) { // operand, even incomplete types. // Same thing in for stmt first clause (when expr) and third clause. - return Owned(static_cast(E)); + return Owned(static_cast(FE.take())); } @@ -598,8 +602,7 @@ Sema::ActOnStartOfSwitchStmt(SourceLocation SwitchLoc, Expr *Cond, Cond = CondResult.take(); if (!CondVar) { - CheckImplicitConversions(Cond, SwitchLoc); - CondResult = MaybeCreateExprWithCleanups(Cond); + CondResult = ActOnFinishFullExpr(Cond, SwitchLoc); if (CondResult.isInvalid()) return StmtError(); Cond = CondResult.take(); @@ -1163,8 +1166,7 @@ Sema::ActOnDoStmt(SourceLocation DoLoc, Stmt *Body, return StmtError(); Cond = CondResult.take(); - CheckImplicitConversions(Cond, DoLoc); - CondResult = MaybeCreateExprWithCleanups(Cond); + CondResult = ActOnFinishFullExpr(Cond, DoLoc); if (CondResult.isInvalid()) return StmtError(); Cond = CondResult.take(); @@ -1442,12 +1444,10 @@ StmtResult Sema::ActOnForEachLValueExpr(Expr *E) { if (result.isInvalid()) return StmtError(); E = result.take(); - CheckImplicitConversions(E); - - result = MaybeCreateExprWithCleanups(E); - if (result.isInvalid()) return StmtError(); - - return Owned(static_cast(result.take())); + ExprResult FullExpr = ActOnFinishFullExpr(E); + if (FullExpr.isInvalid()) + return StmtError(); + return StmtResult(static_cast(FullExpr.take())); } ExprResult @@ -1518,7 +1518,7 @@ Sema::CheckObjCForCollectionOperand(SourceLocation forLoc, Expr *collection) { } // Wrap up any cleanups in the expression. - return Owned(MaybeCreateExprWithCleanups(collection)); + return Owned(collection); } StmtResult @@ -1560,6 +1560,10 @@ Sema::ActOnObjCForCollectionStmt(SourceLocation ForLoc, << FirstType << First->getSourceRange()); } + if (CollectionExprResult.isInvalid()) + return StmtError(); + + CollectionExprResult = ActOnFinishFullExpr(CollectionExprResult.take()); if (CollectionExprResult.isInvalid()) return StmtError(); @@ -2105,9 +2109,13 @@ Sema::ActOnIndirectGotoStmt(SourceLocation GotoLoc, SourceLocation StarLoc, E = ExprRes.take(); if (DiagnoseAssignmentResult(ConvTy, StarLoc, DestTy, ETy, E, AA_Passing)) return StmtError(); - E = MaybeCreateExprWithCleanups(E); } + ExprResult ExprRes = ActOnFinishFullExpr(E); + if (ExprRes.isInvalid()) + return StmtError(); + E = ExprRes.take(); + getCurFunction()->setHasIndirectGoto(); return Owned(new (Context) IndirectGotoStmt(GotoLoc, StarLoc, E)); @@ -2379,8 +2387,10 @@ Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { } if (RetValExp) { - CheckImplicitConversions(RetValExp, ReturnLoc); - RetValExp = MaybeCreateExprWithCleanups(RetValExp); + ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc); + if (ER.isInvalid()) + return StmtError(); + RetValExp = ER.take(); } ReturnStmt *Result = new (Context) ReturnStmt(ReturnLoc, RetValExp, NRVOCandidate); @@ -2482,8 +2492,10 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { } if (RetValExp) { - CheckImplicitConversions(RetValExp, ReturnLoc); - RetValExp = MaybeCreateExprWithCleanups(RetValExp); + ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc); + if (ER.isInvalid()) + return StmtError(); + RetValExp = ER.take(); } } @@ -2541,8 +2553,10 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { } if (RetValExp) { - CheckImplicitConversions(RetValExp, ReturnLoc); - RetValExp = MaybeCreateExprWithCleanups(RetValExp); + ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc); + if (ER.isInvalid()) + return StmtError(); + RetValExp = ER.take(); } Result = new (Context) ReturnStmt(ReturnLoc, RetValExp, NRVOCandidate); } @@ -2592,7 +2606,11 @@ StmtResult Sema::BuildObjCAtThrowStmt(SourceLocation AtLoc, Expr *Throw) { if (Result.isInvalid()) return StmtError(); - Throw = MaybeCreateExprWithCleanups(Result.take()); + Result = ActOnFinishFullExpr(Result.take()); + if (Result.isInvalid()) + return StmtError(); + Throw = Result.take(); + QualType ThrowType = Throw->getType(); // Make sure the expression type is an ObjC pointer or "void *". if (!ThrowType->isDependentType() && @@ -2643,7 +2661,7 @@ Sema::ActOnObjCAtSynchronizedOperand(SourceLocation atLoc, Expr *operand) { } // The operand to @synchronized is a full-expression. - return MaybeCreateExprWithCleanups(operand); + return ActOnFinishFullExpr(operand); } StmtResult diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h index 5400696456..2091cffc83 100644 --- a/lib/Sema/TreeTransform.h +++ b/lib/Sema/TreeTransform.h @@ -2560,7 +2560,7 @@ StmtResult TreeTransform::TransformStmt(Stmt *S) { if (E.isInvalid()) return StmtError(); - return getSema().ActOnExprStmt(getSema().MakeFullExpr(E.take())); + return getSema().ActOnExprStmt(E); } } @@ -5449,7 +5449,7 @@ TreeTransform::TransformForStmt(ForStmt *S) { if (Inc.isInvalid()) return StmtError(); - Sema::FullExprArg FullInc(getSema().MakeFullExpr(Inc.get())); + Sema::FullExprArg FullInc(getSema().MakeFullDiscardedValueExpr(Inc.get())); if (S->getInc() && !FullInc.get()) return StmtError(); diff --git a/test/CXX/temp/temp.decls/temp.variadic/p5.cpp b/test/CXX/temp/temp.decls/temp.variadic/p5.cpp index 726e22227e..945379872f 100644 --- a/test/CXX/temp/temp.decls/temp.variadic/p5.cpp +++ b/test/CXX/temp/temp.decls/temp.variadic/p5.cpp @@ -351,6 +351,15 @@ void test_unexpanded_exprs(Types ...values) { // FIXME: Objective-C expressions will need to go elsewhere for (auto t : values) { } // expected-error{{expression contains unexpanded parameter pack 'values'}} + + switch (values) { } // expected-error{{expression contains unexpanded parameter pack 'values'}} + + do { } while (values); // expected-error{{expression contains unexpanded parameter pack 'values'}} + +test: + goto *values; // expected-error{{expression contains unexpanded parameter pack 'values'}} + + void f(int arg = values); // expected-error{{default argument contains unexpanded parameter pack 'values'}} } // Test unexpanded parameter packs in partial specializations. diff --git a/test/CXX/temp/temp.decls/temp.variadic/p5.mm b/test/CXX/temp/temp.decls/temp.variadic/p5.mm new file mode 100644 index 0000000000..d0598263e5 --- /dev/null +++ b/test/CXX/temp/temp.decls/temp.variadic/p5.mm @@ -0,0 +1,9 @@ +// RUN: %clang_cc1 -fobjc-exceptions -fexceptions -std=c++11 -fblocks -fsyntax-only -verify %s + +template +void f(Types ...values) { + for (id x in values) { } // expected-error {{expression contains unexpanded parameter pack 'values'}} + @synchronized(values) { // expected-error {{expression contains unexpanded parameter pack 'values'}} + @throw values; // expected-error {{expression contains unexpanded parameter pack 'values'}} + } +} -- 2.40.0