ogoffart created this revision.
ogoffart added reviewers: rsmith, cfe-commits.
- When the condition is invalid, replace it by an OpaqueValueExpr
- When parsing an invalid CaseStmt, don't drop the sub statement, just return
it instead.
- In Sema::ActOnStartOfSwitchStmt, always keep the SwitchStmt, even if it has
duplicate case or defaults statement or that the condition cannot be converted
to an integral type.
https://reviews.llvm.org/D26350
Files:
lib/Parse/ParseStmt.cpp
lib/Sema/SemaStmt.cpp
test/Misc/ast-dump-invalid-switch.cpp
Index: test/Misc/ast-dump-invalid-switch.cpp
===================================================================
--- /dev/null
+++ test/Misc/ast-dump-invalid-switch.cpp
@@ -0,0 +1,105 @@
+// RUN: not %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -fms-extensions -ast-dump -ast-dump-filter Test %s | FileCheck -check-prefix CHECK -strict-whitespace %s
+
+/* This test ensures that the AST is still complete, even for invalid code */
+
+namespace TestInvalidSwithCondition {
+int f(int x) {
+ switch (_invalid_) {
+ case 0:
+ return 1;
+ default:
+ return 2;
+ }
+}
+}
+
+// CHECK: NamespaceDecl {{.*}} TestInvalidSwithCondition
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT: |-ParmVarDecl
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT: `-SwitchStmt
+// CHECK-NEXT: |-<<<NULL>>>
+// CHECK-NEXT: |-<<<NULL>>>
+// CHECK-NEXT: |-OpaqueValueExpr
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT: |-CaseStmt
+// CHECK-NEXT: | |-IntegerLiteral {{.*}} 'int' 0
+// CHECK-NEXT: | |-<<<NULL>>>
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: `-DefaultStmt
+// CHECK-NEXT: `-ReturnStmt
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
+
+namespace TestSwitchConditionNotIntegral {
+int g(int *x) {
+ switch (x) {
+ case 0:
+ return 1;
+ default:
+ return 2;
+ }
+}
+}
+
+// CHECK: NamespaceDecl {{.*}} TestSwitchConditionNotIntegral
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT: |-ParmVarDecl
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT: `-SwitchStmt
+// CHECK-NEXT: |-<<<NULL>>>
+// CHECK-NEXT: |-<<<NULL>>>
+// CHECK-NEXT: |-ImplicitCastExpr
+// CHECK-NEXT: | `-DeclRefExpr {{.*}} 'x' 'int *'
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT: |-CaseStmt
+// CHECK-NEXT: | |-IntegerLiteral {{.*}} 'int' 0
+// CHECK-NEXT: | |-<<<NULL>>>
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: `-DefaultStmt
+// CHECK-NEXT: `-ReturnStmt
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
+
+namespace TestSwitchInvalidCases {
+int g(int x) {
+ switch (x) {
+ case _invalid_:
+ return 1;
+ case _invalid_:
+ return 2;
+ case x:
+ return 3;
+ default:
+ return 4;
+ default:
+ return 5;
+ }
+}
+}
+
+// CHECK: NamespaceDecl {{.*}} TestSwitchInvalidCases
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT: |-ParmVarDecl
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT: `-SwitchStmt
+// CHECK-NEXT: |-<<<NULL>>>
+// CHECK-NEXT: |-<<<NULL>>>
+// CHECK-NEXT: |-ImplicitCastExpr
+// CHECK-NEXT: | `-DeclRefExpr {{.*}}'x' 'int'
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT: |-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: |-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK-NEXT: |-CaseStmt
+// CHECK-NEXT: | |-DeclRefExpr {{.*}} 'x' 'int'
+// CHECK-NEXT: | |-<<<NULL>>>
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK-NEXT: |-DefaultStmt
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 4
+// CHECK-NEXT: `-DefaultStmt
+// CHECK-NEXT: `-ReturnStmt
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 5
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -666,7 +666,12 @@
StmtResult Sema::ActOnStartOfSwitchStmt(SourceLocation SwitchLoc,
Stmt *InitStmt, ConditionResult Cond) {
if (Cond.isInvalid())
- return StmtError();
+ Cond = ConditionResult(
+ *this, nullptr,
+ MakeFullExpr(new (Context) OpaqueValueExpr(SourceLocation(),
+ Context.IntTy, VK_RValue),
+ SwitchLoc),
+ false);
getCurFunction()->setHasBranchIntoScope();
@@ -768,7 +773,7 @@
// 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();
+ return SS;
if (CondExpr->isKnownToHaveBooleanValue()) {
// switch(bool_expr) {...} is often a programmer error, e.g.
@@ -1170,11 +1175,6 @@
DiagnoseEmptyStmtBody(CondExpr->getLocEnd(), BodyStmt,
diag::warn_empty_switch_body);
- // FIXME: If the case list was broken is some way, we don't have a good system
- // to patch it up. Instead, just return the whole substmt as broken.
- if (CaseListIsErroneous)
- return StmtError();
-
return SS;
}
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -776,6 +776,9 @@
if (SubStmt.isInvalid())
SubStmt = Actions.ActOnNullStmt(SourceLocation());
Actions.ActOnCaseStmtBody(DeepestParsedCaseStmt, SubStmt.get());
+ } else {
+ // The case statement is invalid, recover by returning the statement body.
+ return SubStmt;
}
// Return the top level parsed statement tree.
@@ -1291,18 +1294,7 @@
StmtResult Switch =
Actions.ActOnStartOfSwitchStmt(SwitchLoc, InitStmt.get(), Cond);
- if (Switch.isInvalid()) {
- // Skip the switch body.
- // FIXME: This is not optimal recovery, but parsing the body is more
- // dangerous due to the presence of case and default statements, which
- // will have no place to connect back with the switch.
- if (Tok.is(tok::l_brace)) {
- ConsumeBrace();
- SkipUntil(tok::r_brace);
- } else
- SkipUntil(tok::semi);
- return Switch;
- }
+ assert(!Switch.isInvalid() && "ActOnStartOfSwitchStmt always succeed");
// C99 6.8.4p3 - In C99, the body of the switch statement is a scope, even if
// there is no compound stmt. C90 does not have this clause. We only do this
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
