https://github.com/jeremy-rifkin created
https://github.com/llvm/llvm-project/pull/123166
This PR updates the CFG building logic and analysis based warning logic to not
consider the default path for `switch` statements to be unreachable when all
enumerators are covered. I.e.:
```cpp
enum class E { a, b };
int foo(E e) {
switch(e) {
case E::a:
return 20;
case E::b:
return 30;
}
// reachable
}
```
The motivation is here is to have `-Wreturn-type` warnings in cases such as
above. Even though it's often sufficient to handle all enumerators, extraneous
enum values are valid and common in the case of flag enums. More context at
#123153. This is in-line with behavior in other compilers (gcc, msvc, and edg
emit warnings for this).
A few test cases broke due to relying on missing returns in the default case.
Two objective C test cases failed, which I would appreciate guidance on so that
I can be sure to test the right things.
On the llvm discord a point was raised about changes to this warning behavior
maybe being disruptive for any codebases that rely heavily on this missing
return analysis behavior. I think the fact so few tests are affected in llvm is
a good sign, and similarly the fact other compilers warn about this makes any
potential disruption limited.
The two relevant commits introducing the enum covering logic:
https://github.com/llvm/llvm-project/commit/50205744c32d3925ba50d1bd69b1c9e6ab3a28b8
https://github.com/llvm/llvm-project/commit/f69ce860487ec0b1eceb23f8391614d5772aae3b
Closes #123153
>From e1ce92c0f54301cacaba316d38d44d20c6d61cb8 Mon Sep 17 00:00:00 2001
From: Jeremy Rifkin <51220084+jeremy-rif...@users.noreply.github.com>
Date: Thu, 16 Jan 2025 00:27:03 -0600
Subject: [PATCH] Don't treat the default switch case as unreachable even when
all enumerators are covered
---
clang/lib/Analysis/CFG.cpp | 12 ++----------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 3 ---
clang/test/Analysis/cfg.cpp | 8 ++++----
...cxx-uninitialized-object-unguarded-access.cpp | 2 ++
clang/test/Sema/return.c | 2 +-
clang/test/SemaCXX/array-bounds.cpp | 16 +++++++---------
6 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 304bbb2b422c61..b819df35189bb8 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -4408,17 +4408,9 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt
*Terminator) {
}
// If we have no "default:" case, the default transition is to the code
- // following the switch body. Moreover, take into account if all the
- // cases of a switch are covered (e.g., switching on an enum value).
- //
- // Note: We add a successor to a switch that is considered covered yet has no
- // case statements if the enumeration has no enumerators.
- bool SwitchAlwaysHasSuccessor = false;
- SwitchAlwaysHasSuccessor |= switchExclusivelyCovered;
- SwitchAlwaysHasSuccessor |= Terminator->isAllEnumCasesCovered() &&
- Terminator->getSwitchCaseList();
+ // following the switch body.
addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock,
- !SwitchAlwaysHasSuccessor);
+ !switchExclusivelyCovered);
// Add the terminator and condition in the switch block.
SwitchTerminatedBlock->setTerminator(Terminator);
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp
b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 589869d0186575..2fa6f0a881e808 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -456,10 +456,7 @@ static ControlFlowKind
CheckFallThrough(AnalysisDeclContext &AC) {
bool HasPlainEdge = false;
bool HasAbnormalEdge = false;
- // Ignore default cases that aren't likely to be reachable because all
- // enums in a switch(X) have explicit case statements.
CFGBlock::FilterOptions FO;
- FO.IgnoreDefaultsWithCoveredEnums = 1;
for (CFGBlock::filtered_pred_iterator I =
cfg->getExit().filtered_pred_start_end(FO);
diff --git a/clang/test/Analysis/cfg.cpp b/clang/test/Analysis/cfg.cpp
index 44a89df28e3b29..940994a7932192 100644
--- a/clang/test/Analysis/cfg.cpp
+++ b/clang/test/Analysis/cfg.cpp
@@ -178,7 +178,7 @@ namespace NoReturnSingleSuccessor {
// CHECK-NEXT: 1: x
// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
// CHECK-NEXT: 3: return [B1.2];
-// CHECK-NEXT: Preds (5): B3 B4 B5 B6 B2(Unreachable)
+// CHECK-NEXT: Preds (5): B3 B4 B5 B6 B2
// CHECK-NEXT: Succs (1): B0
// CHECK: [B2]
// CHECK-NEXT: 1: 0
@@ -188,7 +188,7 @@ namespace NoReturnSingleSuccessor {
// CHECK-NEXT: 5: [B2.4] (ImplicitCastExpr, IntegralCast, int)
// CHECK-NEXT: T: switch [B2.5]
// CHECK-NEXT: Preds (1): B7
-// CHECK-NEXT: Succs (5): B3 B4 B5 B6 B1(Unreachable)
+// CHECK-NEXT: Succs (5): B3 B4 B5 B6 B1
// CHECK: [B3]
// CHECK-NEXT: case D:
// CHECK-NEXT: 1: 4
@@ -254,14 +254,14 @@ int test_enum_with_extension(enum MyEnum value) {
// CHECK-NEXT: 5: [B2.4] (ImplicitCastExpr, IntegralCast, int)
// CHECK-NEXT: T: switch [B2.5]
// CHECK-NEXT: Preds (1): B7
-// CHECK-NEXT: Succs (4): B4 B5 B6 B3(Unreachable)
+// CHECK-NEXT: Succs (4): B4 B5 B6 B3
// CHECK: [B3]
// CHECK-NEXT: default:
// CHECK-NEXT: 1: 4
// CHECK-NEXT: 2: x
// CHECK-NEXT: 3: [B3.2] = [B3.1]
// CHECK-NEXT: T: break;
-// CHECK-NEXT: Preds (1): B2(Unreachable)
+// CHECK-NEXT: Preds (1): B2
// CHECK-NEXT: Succs (1): B1
// CHECK: [B4]
// CHECK-NEXT: case C:
diff --git a/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
b/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
index 611e1d8255976c..b791c527c2e33e 100644
--- a/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
@@ -383,6 +383,7 @@ class SwitchGuardedFieldsTest {
case Kind::V:
return -1;
}
+ halt();
}
int operator+() {
@@ -392,6 +393,7 @@ class SwitchGuardedFieldsTest {
case Kind::V:
return -1;
}
+ halt();
}
};
diff --git a/clang/test/Sema/return.c b/clang/test/Sema/return.c
index 14d5300e819492..b0814f152cbe95 100644
--- a/clang/test/Sema/return.c
+++ b/clang/test/Sema/return.c
@@ -265,7 +265,7 @@ int test_enum_cases(enum Cases C) {
case C4: return 3;
case C3: return 4;
}
-} // no-warning
+} // expected-warning {{non-void function does not return a value in all
control paths}}
// PR12318 - Don't give a may reach end of non-void function warning.
int test34(int x) {
diff --git a/clang/test/SemaCXX/array-bounds.cpp
b/clang/test/SemaCXX/array-bounds.cpp
index b584e1e7cd4537..3889a4bd0f4687 100644
--- a/clang/test/SemaCXX/array-bounds.cpp
+++ b/clang/test/SemaCXX/array-bounds.cpp
@@ -133,7 +133,7 @@ int test_pr9296() {
int test_sizeof_as_condition(int flag) {
int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
- if (flag)
+ if (flag)
return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning
{{array index 2 is past the end of the array (that has type 'int[2]')}}
}
@@ -170,16 +170,14 @@ void test_nested_switch() {
}
}
-// Test that if all the values of an enum covered, that the 'default' branch
-// is unreachable.
+// Test that a warning is not emitted if the code is unreachable.
enum Values { A, B, C, D };
void test_all_enums_covered(enum Values v) {
int x[2];
- switch (v) {
- case A: return;
- case B: return;
- case C: return;
- case D: return;
+ if(v == A) {
+ return;
+ } else {
+ return;
}
x[2] = 0; // no-warning
}
@@ -244,7 +242,7 @@ void test_pr10771() {
}
int test_pr11007_aux(const char * restrict, ...);
-
+
// Test checking with varargs.
void test_pr11007() {
double a[5]; // expected-note {{array 'a' declared here}}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
