Refactor checking of switch conditions and case values.
Check each case value in turn while parsing it, performing the
conversion to the switch type within the context of the expression
itself. This will become necessary in order to properly handle cleanups
for temporaries created as part of the case label (in an upcoming
patch). For now it's just good hygiene.
This necessitates moving the checking for the switch condition itself to
earlier, so that the destination type is available when checking the
case labels.
As a nice side-effect, we get slightly improved diagnostic quality and
error recovery by separating the case expression checking from the case
statement checking and from tracking whether there are discarded case
labels.
llvm-svn: 338056
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 058a2cd..b2f9783 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -392,65 +392,79 @@
return CompoundStmt::Create(Context, Elts, L, R);
}
+ExprResult
+Sema::ActOnCaseExpr(SourceLocation CaseLoc, ExprResult Val) {
+ if (!Val.get())
+ return Val;
+
+ if (DiagnoseUnexpandedParameterPack(Val.get()))
+ return ExprError();
+
+ // If we're not inside a switch, let the 'case' statement handling diagnose
+ // this. Just clean up after the expression as best we can.
+ if (!getCurFunction()->SwitchStack.empty()) {
+ Expr *CondExpr =
+ getCurFunction()->SwitchStack.back().getPointer()->getCond();
+ if (!CondExpr)
+ return ExprError();
+ QualType CondType = CondExpr->getType();
+
+ auto CheckAndFinish = [&](Expr *E) {
+ if (CondType->isDependentType() || E->isTypeDependent())
+ return ExprResult(E);
+
+ if (getLangOpts().CPlusPlus11) {
+ // C++11 [stmt.switch]p2: the constant-expression shall be a converted
+ // constant expression of the promoted type of the switch condition.
+ llvm::APSInt TempVal;
+ return CheckConvertedConstantExpression(E, CondType, TempVal,
+ CCEK_CaseValue);
+ }
+
+ ExprResult ER = E;
+ if (!E->isValueDependent())
+ ER = VerifyIntegerConstantExpression(E);
+ if (!ER.isInvalid())
+ ER = DefaultLvalueConversion(ER.get());
+ if (!ER.isInvalid())
+ ER = ImpCastExprToType(ER.get(), CondType, CK_IntegralCast);
+ return ER;
+ };
+
+ ExprResult Converted = CorrectDelayedTyposInExpr(Val, CheckAndFinish);
+ if (Converted.get() == Val.get())
+ Converted = CheckAndFinish(Val.get());
+ if (Converted.isInvalid())
+ return ExprError();
+ Val = Converted;
+ }
+
+ return ActOnFinishFullExpr(Val.get(), Val.get()->getExprLoc(), false,
+ getLangOpts().CPlusPlus11);
+}
+
StmtResult
-Sema::ActOnCaseStmt(SourceLocation CaseLoc, Expr *LHSVal,
- SourceLocation DotDotDotLoc, Expr *RHSVal,
+Sema::ActOnCaseStmt(SourceLocation CaseLoc, ExprResult LHSVal,
+ SourceLocation DotDotDotLoc, ExprResult RHSVal,
SourceLocation ColonLoc) {
- assert(LHSVal && "missing expression in case statement");
+ assert((LHSVal.isInvalid() || LHSVal.get()) && "missing LHS value");
+ assert((DotDotDotLoc.isInvalid() ? RHSVal.isUnset()
+ : RHSVal.isInvalid() || RHSVal.get()) &&
+ "missing RHS value");
if (getCurFunction()->SwitchStack.empty()) {
Diag(CaseLoc, diag::err_case_not_in_switch);
return StmtError();
}
- ExprResult LHS =
- CorrectDelayedTyposInExpr(LHSVal, [this](class Expr *E) {
- if (!getLangOpts().CPlusPlus11)
- return VerifyIntegerConstantExpression(E);
- if (Expr *CondExpr =
- getCurFunction()->SwitchStack.back()->getCond()) {
- QualType CondType = CondExpr->getType();
- llvm::APSInt TempVal;
- return CheckConvertedConstantExpression(E, CondType, TempVal,
- CCEK_CaseValue);
- }
- return ExprError();
- });
- if (LHS.isInvalid())
+ if (LHSVal.isInvalid() || RHSVal.isInvalid()) {
+ getCurFunction()->SwitchStack.back().setInt(true);
return StmtError();
- LHSVal = LHS.get();
-
- if (!getLangOpts().CPlusPlus11) {
- // C99 6.8.4.2p3: The expression shall be an integer constant.
- // However, GCC allows any evaluatable integer expression.
- if (!LHSVal->isTypeDependent() && !LHSVal->isValueDependent()) {
- LHSVal = VerifyIntegerConstantExpression(LHSVal).get();
- if (!LHSVal)
- return StmtError();
- }
-
- // GCC extension: The expression shall be an integer constant.
-
- if (RHSVal && !RHSVal->isTypeDependent() && !RHSVal->isValueDependent()) {
- RHSVal = VerifyIntegerConstantExpression(RHSVal).get();
- // Recover from an error by just forgetting about it.
- }
}
- LHS = ActOnFinishFullExpr(LHSVal, LHSVal->getExprLoc(), false,
- getLangOpts().CPlusPlus11);
- if (LHS.isInvalid())
- return StmtError();
-
- auto RHS = RHSVal ? ActOnFinishFullExpr(RHSVal, RHSVal->getExprLoc(), false,
- getLangOpts().CPlusPlus11)
- : ExprResult();
- if (RHS.isInvalid())
- return StmtError();
-
CaseStmt *CS = new (Context)
- CaseStmt(LHS.get(), RHS.get(), CaseLoc, DotDotDotLoc, ColonLoc);
- getCurFunction()->SwitchStack.back()->addSwitchCase(CS);
+ CaseStmt(LHSVal.get(), RHSVal.get(), CaseLoc, DotDotDotLoc, ColonLoc);
+ getCurFunction()->SwitchStack.back().getPointer()->addSwitchCase(CS);
return CS;
}
@@ -473,7 +487,7 @@
}
DefaultStmt *DS = new (Context) DefaultStmt(DefaultLoc, ColonLoc, SubStmt);
- getCurFunction()->SwitchStack.back()->addSwitchCase(DS);
+ getCurFunction()->SwitchStack.back().getPointer()->addSwitchCase(DS);
return DS;
}
@@ -679,20 +693,44 @@
if (CondResult.isInvalid())
return ExprError();
+ // FIXME: PerformContextualImplicitConversion doesn't always tell us if it
+ // failed and produced a diagnostic.
+ Cond = CondResult.get();
+ if (!Cond->isTypeDependent() &&
+ !Cond->getType()->isIntegralOrEnumerationType())
+ return ExprError();
+
// C99 6.8.4.2p5 - Integer promotions are performed on the controlling expr.
- return UsualUnaryConversions(CondResult.get());
+ return UsualUnaryConversions(Cond);
}
StmtResult Sema::ActOnStartOfSwitchStmt(SourceLocation SwitchLoc,
Stmt *InitStmt, ConditionResult Cond) {
- if (Cond.isInvalid())
- return StmtError();
+ Expr *CondExpr = Cond.get().second;
+ assert((Cond.isInvalid() || CondExpr) && "switch with no condition");
+
+ if (CondExpr && !CondExpr->isTypeDependent()) {
+ // We have already converted the expression to an integral or enumeration
+ // type, when we parsed the switch condition. If we don't have an
+ // appropriate type now, enter the switch scope but remember that it's
+ // invalid.
+ assert(CondExpr->getType()->isIntegralOrEnumerationType() &&
+ "invalid condition type");
+ if (CondExpr->isKnownToHaveBooleanValue()) {
+ // switch(bool_expr) {...} is often a programmer error, e.g.
+ // switch(n && mask) { ... } // Doh - should be "n & mask".
+ // One can always use an if statement instead of switch(bool_expr).
+ Diag(SwitchLoc, diag::warn_bool_switch_condition)
+ << CondExpr->getSourceRange();
+ }
+ }
setFunctionHasBranchIntoScope();
SwitchStmt *SS = new (Context)
- SwitchStmt(Context, InitStmt, Cond.get().first, Cond.get().second);
- getCurFunction()->SwitchStack.push_back(SS);
+ SwitchStmt(Context, InitStmt, Cond.get().first, CondExpr);
+ getCurFunction()->SwitchStack.push_back(
+ FunctionScopeInfo::SwitchInfo(SS, false));
return SS;
}
@@ -705,6 +743,10 @@
/// type.
static void checkCaseValue(Sema &S, SourceLocation Loc, const llvm::APSInt &Val,
unsigned UnpromotedWidth, bool UnpromotedSign) {
+ // In C++11 onwards, this is checked by the language rules.
+ if (S.getLangOpts().CPlusPlus11)
+ return;
+
// If the case value was signed and negative and the switch expression is
// unsigned, don't bother to warn: this is implementation-defined behavior.
// FIXME: Introduce a second, default-ignored warning for this case?
@@ -759,7 +801,7 @@
static void checkEnumTypesInSwitchStmt(Sema &S, const Expr *Cond,
const Expr *Case) {
- QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
+ QualType CondType = Cond->getType();
QualType CaseType = Case->getType();
const EnumType *CondEnumType = CondType->getAs<EnumType>();
@@ -787,7 +829,8 @@
Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
Stmt *BodyStmt) {
SwitchStmt *SS = cast<SwitchStmt>(Switch);
- assert(SS == getCurFunction()->SwitchStack.back() &&
+ bool CaseListIsIncomplete = getCurFunction()->SwitchStack.back().getInt();
+ assert(SS == getCurFunction()->SwitchStack.back().getPointer() &&
"switch stack missing push/pop!");
getCurFunction()->SwitchStack.pop_back();
@@ -800,10 +843,6 @@
QualType CondType = CondExpr->getType();
- const Expr *CondExprBeforePromotion = CondExpr;
- QualType CondTypeBeforePromotion =
- GetTypeBeforeIntegralPromotion(CondExprBeforePromotion);
-
// C++ 6.4.2.p2:
// Integral promotions are performed (on the switch condition).
//
@@ -811,21 +850,9 @@
// type (before the promotion) doesn't make sense, even when it can
// be represented by the promoted type. Therefore we need to find
// the pre-promotion type of the switch condition.
- if (!CondExpr->isTypeDependent()) {
- // We have already converted the expression to an integral or enumeration
- // type, when we started the switch statement. If we don't have an
- // appropriate type now, just return an error.
- if (!CondType->isIntegralOrEnumerationType())
- return StmtError();
-
- if (CondExpr->isKnownToHaveBooleanValue()) {
- // switch(bool_expr) {...} is often a programmer error, e.g.
- // switch(n && mask) { ... } // Doh - should be "n & mask".
- // One can always use an if statement instead of switch(bool_expr).
- Diag(SwitchLoc, diag::warn_bool_switch_condition)
- << CondExpr->getSourceRange();
- }
- }
+ const Expr *CondExprBeforePromotion = CondExpr;
+ QualType CondTypeBeforePromotion =
+ GetTypeBeforeIntegralPromotion(CondExprBeforePromotion);
// Get the bitwidth of the switched-on value after promotions. We must
// convert the integer case values to this width before comparison.
@@ -878,50 +905,32 @@
Expr *Lo = CS->getLHS();
- if (Lo->isTypeDependent() || Lo->isValueDependent()) {
+ if (Lo->isValueDependent()) {
HasDependentValue = true;
break;
}
- checkEnumTypesInSwitchStmt(*this, CondExpr, Lo);
-
- llvm::APSInt LoVal;
-
- if (getLangOpts().CPlusPlus11) {
- // C++11 [stmt.switch]p2: the constant-expression shall be a converted
- // constant expression of the promoted type of the switch condition.
- ExprResult ConvLo =
- CheckConvertedConstantExpression(Lo, CondType, LoVal, CCEK_CaseValue);
- if (ConvLo.isInvalid()) {
- CaseListIsErroneous = true;
- continue;
- }
- Lo = ConvLo.get();
- } else {
- // We already verified that the expression has a i-c-e value (C99
- // 6.8.4.2p3) - get that value now.
- LoVal = Lo->EvaluateKnownConstInt(Context);
-
- // If the LHS is not the same type as the condition, insert an implicit
- // cast.
- Lo = DefaultLvalueConversion(Lo).get();
- Lo = ImpCastExprToType(Lo, CondType, CK_IntegralCast).get();
- }
+ // We already verified that the expression has a constant value;
+ // get that value (prior to conversions).
+ const Expr *LoBeforePromotion = Lo;
+ GetTypeBeforeIntegralPromotion(LoBeforePromotion);
+ llvm::APSInt LoVal = LoBeforePromotion->EvaluateKnownConstInt(Context);
// Check the unconverted value is within the range of possible values of
// the switch expression.
checkCaseValue(*this, Lo->getLocStart(), LoVal,
CondWidthBeforePromotion, CondIsSignedBeforePromotion);
+ // FIXME: This duplicates the check performed for warn_not_in_enum below.
+ checkEnumTypesInSwitchStmt(*this, CondExprBeforePromotion,
+ LoBeforePromotion);
+
// Convert the value to the same width/sign as the condition.
AdjustAPSInt(LoVal, CondWidth, CondIsSigned);
- CS->setLHS(Lo);
-
// If this is a case range, remember it in CaseRanges, otherwise CaseVals.
if (CS->getRHS()) {
- if (CS->getRHS()->isTypeDependent() ||
- CS->getRHS()->isValueDependent()) {
+ if (CS->getRHS()->isValueDependent()) {
HasDependentValue = true;
break;
}
@@ -1002,27 +1011,10 @@
llvm::APSInt &LoVal = CaseRanges[i].first;
CaseStmt *CR = CaseRanges[i].second;
Expr *Hi = CR->getRHS();
- llvm::APSInt HiVal;
- if (getLangOpts().CPlusPlus11) {
- // C++11 [stmt.switch]p2: the constant-expression shall be a converted
- // constant expression of the promoted type of the switch condition.
- ExprResult ConvHi =
- CheckConvertedConstantExpression(Hi, CondType, HiVal,
- CCEK_CaseValue);
- if (ConvHi.isInvalid()) {
- CaseListIsErroneous = true;
- continue;
- }
- Hi = ConvHi.get();
- } else {
- HiVal = Hi->EvaluateKnownConstInt(Context);
-
- // If the RHS is not the same type as the condition, insert an
- // implicit cast.
- Hi = DefaultLvalueConversion(Hi).get();
- Hi = ImpCastExprToType(Hi, CondType, CK_IntegralCast).get();
- }
+ const Expr *HiBeforePromotion = Hi;
+ GetTypeBeforeIntegralPromotion(HiBeforePromotion);
+ llvm::APSInt HiVal = HiBeforePromotion->EvaluateKnownConstInt(Context);
// Check the unconverted value is within the range of possible values of
// the switch expression.
@@ -1032,8 +1024,6 @@
// Convert the value to the same width/sign as the condition.
AdjustAPSInt(HiVal, CondWidth, CondIsSigned);
- CR->setRHS(Hi);
-
// If the low value is bigger than the high value, the case is empty.
if (LoVal > HiVal) {
Diag(CR->getLHS()->getLocStart(), diag::warn_case_empty_range)
@@ -1104,7 +1094,8 @@
}
// Complain if we have a constant condition and we didn't find a match.
- if (!CaseListIsErroneous && ShouldCheckConstantCond) {
+ if (!CaseListIsErroneous && !CaseListIsIncomplete &&
+ ShouldCheckConstantCond) {
// TODO: it would be nice if we printed enums as enums, chars as
// chars, etc.
Diag(CondExpr->getExprLoc(), diag::warn_missing_case_for_condition)
@@ -1120,8 +1111,8 @@
const EnumType *ET = CondTypeBeforePromotion->getAs<EnumType>();
// If switch has default case, then ignore it.
- if (!CaseListIsErroneous && !HasConstantCond && ET &&
- ET->getDecl()->isCompleteDefinition()) {
+ if (!CaseListIsErroneous && !CaseListIsIncomplete && !HasConstantCond &&
+ ET && ET->getDecl()->isCompleteDefinition()) {
const EnumDecl *ED = ET->getDecl();
EnumValsTy EnumVals;