-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][Sema] Add -Wswitch-default warning option #73077
Conversation
@llvm/pr-subscribers-clang Author: dong jianqiang (dongjianqiang2) ChangesAdds a warning, issued by the clang semantic analysis. The patch warns on switch which don't have the default branch. This is a counterpart of gcc's Wswitch-default. Full diff: https://github.com/llvm/llvm-project/pull/73077.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index ff028bbbf74261e..12b11527b30571a 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -632,7 +632,7 @@ def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor,
def Shorten64To32 : DiagGroup<"shorten-64-to-32">;
def : DiagGroup<"sign-promo">;
def SignCompare : DiagGroup<"sign-compare">;
-def : DiagGroup<"switch-default">;
+def SwitchDefault : DiagGroup<"switch-default">;
def : DiagGroup<"synth">;
def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 990692c06d7d3a8..17c9627910bb6ce 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10044,6 +10044,8 @@ def warn_missing_case : Warning<"%plural{"
"3:enumeration values %1, %2, and %3 not handled in switch|"
":%0 enumeration values not handled in switch: %1, %2, %3...}0">,
InGroup<Switch>;
+def warn_switch_default : Warning<"switch missing default case">,
+ InGroup<SwitchDefault>, DefaultIgnore;
def warn_unannotated_fallthrough : Warning<
"unannotated fall-through between switch labels">,
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 2b45aa5dff7be7c..63348d27a8c94a1 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1327,6 +1327,9 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
}
}
+ if (!TheDefaultStmt)
+ Diag(SwitchLoc, diag::warn_switch_default);
+
if (!HasDependentValue) {
// If we don't have a default statement, check whether the
// condition is constant.
diff --git a/clang/test/Sema/switch-default.c b/clang/test/Sema/switch-default.c
new file mode 100644
index 000000000000000..3f2e21693303378
--- /dev/null
+++ b/clang/test/Sema/switch-default.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-default %s
+
+int f1(int a) {
+ switch (a) { // expected-warning {{switch missing default case}}
+ case 1: a++; break;
+ case 2: a += 2; break;
+ }
+ return a;
+}
+
+int f2(int a) {
+ switch (a) { // no-warning
+ default:
+ ;
+ }
+ return a;
+}
|
There is one clang-tidy check (bugprone-switch-missing-default-case) also for this feature. |
Thank you for your reply. It may be a more convenient and straightforward way if can be identified during compile time. |
I'd like other to weight in on that but even though GCC has that flag (which clang recognizes and ignores), I'm, a bit reluctant to add a warning for it. For example, Folks at google advise against using LLVM development heavily relies on Maybe the existing tidy check is the right tool for this. |
I see it's easy to implement in LLVM like clang tidy (not warning for enum and constant expr). The clang tidy may seperate the IMAO, if we can implement it together in compiler, why not? |
@AaronBallman @erichkeane @shafik WDYT? |
This excludes enumeration types though, while GCC
Yes. Though I have to say, even though I'm squarely in the I understand the policy against off-by-default warnings to avoid crowding the core compiler, but this is essentially just two lines of code, and if GCC has it we're not going out on a limb here. And since we already seem to support the flag for GCC compatibility, we might as well make it work if it's that easy.
I think we're using |
In the end it probably boils down to how you view an enumeration type. Is it a type with the declared enumerators as inhabitants, or is it an integer type of some length with associated symbolic constants of that type? In other words, is an enumeration a "strong" type or a "weak" type? The standard itself seems more in the "weak" camp. Yes, it does not allow integer values larger than what the largest enumeration value requires (if the underlying integer type isn't declared), but otherwise integer values that don't correspond to enumeration values can be cast to the enumeration type just fine. In [dcl.enum]p8:
However, nothing prevents a developer from assuming a stricter model, where only the declared enumeration values are allowed and everything else is undefined behavior. (If not in terms of the language, then in terms of the program.) That is what we have done in the LLVM code base, and it's also the model that I personally prefer. But it is probably legitimate to follow the weaker model implied by the standard and assume that enumerations can have non-enumerator values, which requires a default label, enforced by this warning. |
I think there is value to adding this if only for the GCC compat story here. I realize we don't like non-on-by-default warnings, but it IS something that folks use reasonably often, and we've even acquiesced to making it an ignored flag IIRC. Aaron is the real decision maker here, but I think I'd have no problem holding my nose and allowing the non-on-by-default here, if only to match GCC. BUT, this should make sure to match behavior of GCC WRT enums/etc. But if folks want a warning for a fully-covered enum, I think there is value in giving it to them. Patch itself need some additional testing (particularly around templates/dependence/etc), a release note, additional documentation in our commandline reference/etc. |
Specifically @AaronBallman, not me. |
@@ -10044,6 +10044,8 @@ def warn_missing_case : Warning<"%plural{" | |||
"3:enumeration values %1, %2, and %3 not handled in switch|" | |||
":%0 enumeration values not handled in switch: %1, %2, %3...}0">, | |||
InGroup<Switch>; | |||
def warn_switch_default : Warning<"switch missing default case">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def warn_switch_default : Warning<"switch missing default case">, | |
def warn_switch_default : Warning<"'switch' missing 'default' label">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the correction!
I think GCC compatibility is sufficient reason to support this despite being off-by-default and basically no hope of ever enabling it by default. There's evidence that people enable this in the wild, so I think it meets the bar: https://sourcegraph.com/search?q=context%3Aglobal+-Wswitch-default+-file%3A.*clang.*+-file%3A.*gcc.*&patternType=standard&sm=1&groupBy=repo |
Adds a warning, issued by the clang semantic analysis. The patch warns on switch which don't have the default branch. This is a counterpart of gcc's Wswitch-default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Revert: breaks MIOpen https://ontrack-internal.amd.com/browse/SWDEV-436625 [clang][Sema] Add -Wswitch-default warning option (llvm#73077) Change-Id: I182ea84b40257240ed42db5cd28213ab192ac3f7
report: how to solve for enum class? |
Hi, -Wswitch-default is a warning option on switch which don't have the default branch, the option is disabled by default, You can check whether -Wswitch-default is added in the build script. |
case 2: a += 2; break; | ||
} | ||
return a; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding a test case that shows that this warns even for completely covered switches e.g. https://godbolt.org/z/zbqr6P5xc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PR(#75900 )
In real projects mix enum class with old int switch. Try to update defaults and have noise from enum class case. |
Using an enum class without default is a good opportunity to get a warning in all places of use in the switch construct when adding new elements to the enum |
@gendalph, this warning is meant to always warn if a |
[llvm#73077] added Wswitch-default diagnostic but it produced false positives in templates. This PR address it.
Adds a warning, issued by the clang semantic analysis. The patch warns on switch which don't have the default branch. This is a counterpart of gcc's Wswitch-default. Change-Id: Iba221a303560b88fd7e408a517cfba2d728b0931
This test case exists upstream currently. It was deleted by mistake when reapplying the patch: [clang][Sema] Add -Wswitch-default warning option (llvm#73077) The test case had been added upstream for a subsequent patch [Clang][Sema] Fix Wswitch-default bad warning in template (llvm#76007) Change-Id: I94fdbe2daa4703166e0d7fc00a3a4d08f98ae410
This reverts commit 2cf433b. Change-Id: I630da1ecf57cb003cc3062252b39e9e2b9ddf7d1
llvm/llvm-project#73077 added -Wswitch-default diagnostic but it produced false positives in templates. This PR will address that. llvm/llvm-project#75943
Adds a warning, issued by the clang semantic analysis. The patch warns on switch which don't have the default branch.
This is a counterpart of gcc's Wswitch-default.