[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 accepted this revision. xbolva00 added a comment. This revision is now accepted and ready to land. Since motivation examples are already handled by -Wunreachable-code, I will close this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

jfb added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192 +def warn_unreachable_stmt_in_switch : Warning< + "statement will be never executed">, InGroup>; def warn_bool_switch_condition : Warning< aaron.ballman wrote: > I thought w

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

aaron.ballman added a comment. In D63139#1603667 , @xbolva00 wrote: > Ping again > > Is ‘CaseStmt´ AST C++ issue is a blocker for this patch or not? I do not think it is a blocker for this patch. It's the existing behavior of the AST and while annoying,

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 added a subscriber: jfb. xbolva00 added a comment. Adding @jfb as reviewer to get more opinions what should be done next.. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org/D63139 ___ cfe-commits mai

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 added a comment. Ping again Is ‘CaseStmt´ AST C++ issue is a blocker for this patch or not? I would like to move forward - either land it or (if blocker) abandon it. If it is blocking issue, I can enable it for C only (better than nothing).. CHANGES SINCE LAST ACTION https://review

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 added a comment. Ping @rsmith CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org/D63139 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

rjmccall added a comment. I agree that we really shouldn't use most of those — we shouldn't have null types or null child expressions anywhere in the AST. (Accessors might return null for *optional* children, of course, but that's different.) And yeah, a big part of that would be having an er

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

aaron.ballman added a comment. In D63139#1579231 , @rjmccall wrote: > I agree that tools shouldn't be forced to deal with invalid AST that looks > like valid AST. To me that means finding ways to preserve information that > (1) don't badly violate invar

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

rjmccall added a comment. I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid. For `case`, which has external require

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

aaron.ballman added a comment. In D63139#1561891 , @rjmccall wrote: > Are the `CaseStmt`s being dropped in C++ because the expression overflows? I > agree that that's bad AST behavior; we should strive to generate AST whenever > we can, even if it's not

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 added a comment. Yes, CaseStmt is dropped in that case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org/D63139 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

rjmccall added a comment. Are the `CaseStmt`s being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://review

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 added a comment. In D63139#1547968 , @xbolva00 wrote: > There is a bug in Clang AST. @rsmith > > Check test/Sema/switch-1.c. In C++ mode we have really bad AST: > > CompoundStmt 0x55f5b7d68460 > | | |-ReturnStmt 0x55f5b7d68060 > | |

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 updated this revision to Diff 205295. xbolva00 added a comment. Addressed some notes. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org/D63139 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaStmt.cpp test/Sema/implic

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 added a comment. There is a bug in Clang AST. @rsmith Check testcase test/Sema/switch-1.c. In C++ mode we have AST: CompoundStmt 0x55f5b7d68460 | | | -ReturnStmt 0x55f5b7d68060| | |

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:541 def SwitchEnum : DiagGroup<"switch-enum">; +def SwitchUnreachable : DiagGroup<"switch-unreachable">; def Switch : DiagGroup<"switch">; I don't think you

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 updated this revision to Diff 204254. xbolva00 added a comment. Attached forgotten tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org/D63139 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 updated this revision to Diff 204253. xbolva00 added a comment. Warn in more cases. Added many new tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://reviews.llvm.org/D63139 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticS

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

xbolva00 created this revision. xbolva00 added reviewers: aaron.ballman, rsmith, rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. Detects unreachable statements in switches. GCC supports this warning too, on by default. Repository: rC Clang https://reviews.llv