Page MenuHomePhabricator

[Diagnostics] Implement -Wswitch-unreachable
Needs ReviewPublic

Authored by xbolva00 on Tue, Jun 11, 7:12 AM.

Details

Summary

Detects unreachable statements in switches. GCC supports this warning too, on by default.

Diff Detail

Event Timeline

xbolva00 created this revision.Tue, Jun 11, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 11, 7:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 updated this revision to Diff 204253.Wed, Jun 12, 3:57 AM

Warn in more cases. Added many new tests.

xbolva00 updated this revision to Diff 204254.Wed, Jun 12, 3:59 AM

Attached forgotten tests

aaron.ballman added inline comments.Mon, Jun 17, 10:56 AM
include/clang/Basic/DiagnosticGroups.td
541 ↗(On Diff #204254)

I don't think you need this group because there's only one diagnostic it controls. You can add the group directly to the warning itself.

include/clang/Basic/DiagnosticSemaKinds.td
8192

InGroup<DiagGroup<"switch-unreachable">> and drop the DefaultIgnore.

lib/Sema/SemaStmt.cpp
880

const auto *

881–882

for (const Stmt *S : CS->body())

887–892

You should turn this into a negative predicate rather than having an empty body with an else statement.

test/Sema/switch_unreachable.c
2 ↗(On Diff #204254)

Remove the -Wall since you want to test that this is on by default.

xbolva00 added a comment.EditedTue, Jun 18, 3:20 AM

There is a bug in Clang AST. @rsmith

Check test/Sema/switch-1.c. In C++ mode we have really bad AST:

CompoundStmt 0x55f5b7d68460 <col:14, line:51:3>
|   |   |-ReturnStmt 0x55f5b7d68060 <line:17:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68040 <col:14> 'int' 1
|   |   |-ReturnStmt 0x55f5b7d68120 <line:25:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68100 <col:14> 'int' 2
|   |   |-ReturnStmt 0x55f5b7d68210 <line:33:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d681f0 <col:14> 'int' 3
|   |   |-ReturnStmt 0x55f5b7d683c0 <line:48:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d683a0 <col:14> 'int' 4
|   |   `-CaseStmt 0x55f5b7d68408 <line:49:5, line:50:14>
|   |     |-ConstantExpr 0x55f5b7d683f0 <line:49:10> 'int'
|   |     | `-IntegerLiteral 0x55f5b7d683d0 <col:10> 'int' 2147483647
|   |     `-ReturnStmt 0x55f5b7d68450 <line:50:7, col:14>
|   |       `-IntegerLiteral 0x55f5b7d68430 <col:14> 'int' 0

This is bad and blocks this patch since it creates weird warnings :/

xbolva00 updated this revision to Diff 205295.Tue, Jun 18, 3:49 AM

Addressed some notes. Thanks.

There is a bug in Clang AST. @rsmith

Check test/Sema/switch-1.c. In C++ mode we have really bad AST:

CompoundStmt 0x55f5b7d68460 <col:14, line:51:3>
|   |   |-ReturnStmt 0x55f5b7d68060 <line:17:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68040 <col:14> 'int' 1
|   |   |-ReturnStmt 0x55f5b7d68120 <line:25:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68100 <col:14> 'int' 2
|   |   |-ReturnStmt 0x55f5b7d68210 <line:33:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d681f0 <col:14> 'int' 3
|   |   |-ReturnStmt 0x55f5b7d683c0 <line:48:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d683a0 <col:14> 'int' 4
|   |   `-CaseStmt 0x55f5b7d68408 <line:49:5, line:50:14>
|   |     |-ConstantExpr 0x55f5b7d683f0 <line:49:10> 'int'
|   |     | `-IntegerLiteral 0x55f5b7d683d0 <col:10> 'int' 2147483647
|   |     `-ReturnStmt 0x55f5b7d68450 <line:50:7, col:14>
|   |       `-IntegerLiteral 0x55f5b7d68430 <col:14> 'int' 0

This is bad and blocks this patch since it creates weird warnings :/

Or... is it acceptable to produce unreachable stmt warning for this cases?