Page MenuHomePhabricator

[clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

Authored by JonasToth on Sep 13 2017, 6:51 AM.



This check searches for missing else branches in if-else if-chains and
missing default labels in switch statements, that use integers as condition.

It is very similar to -Wswitch, but concentrates on integers only, since enums are
already covered.

The option to warn for missing else branches is deactivated by default, since it is
very noise on larger code bases.

Running it on LLVM:

for default configuration
just for llvm/lib/Analysis/ScalarEvolution.cpp, the else-path checker is very noisy!

Diff Detail


Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth added inline comments.Oct 13 2017, 12:02 PM
96 ↗(On Diff #118911)

Woops. I mixed codeplaces (since bad duplication from my side). I did merge the diagnostics better, making this a natural fit for a switch.

152 ↗(On Diff #118911)

I kinda rewrote the whole checking part.
Updated diff comes in a sec. I found that bitfields are not handled as they should.

JonasToth marked 4 inline comments as done.Oct 13 2017, 12:03 PM

Improved check a lot. Hope reviewers now have an easier time reading it.

Example output for llvm/lib/Target/X86
Running it over the whole llvm/lib codebase generates a lot of warnings. Please note, that it seems to be common to write code like this:

int Val;
switch(Val) {
  case 1: // something
  case 2: // something else
  case 16: // magic

In some cases it has been taken care that no fallthrough happens, but it's not so simple to spot. Using default: llvm_unreachable("reason"); would comply with the check.

JDevlieghere added inline comments.Oct 16 2017, 8:18 AM
76 ↗(On Diff #118951)

Add a comment describing what this function does. I'd move and rephrase the comment below.

98 ↗(On Diff #118951)

Unless you expect a whole bunch of logic to be added here, I'd un-const and initialize BitCount to zero, then just have if-clause reassign it and get rid of the lambda. This will save you a few lines of code and complexity.

149 ↗(On Diff #118951)

If there's only going to be two messages, you could use the ternary operator and save an instantiation of the SmallVector.

195 ↗(On Diff #118951)

Let's move this to right after where you define CaseCount

JonasToth marked 4 inline comments as done.Oct 18 2017, 1:36 AM
JonasToth added inline comments.
98 ↗(On Diff #118951)

I dont know if i missed some integral types, that need special care, so this one was defensive.

But with the refactoring i made, I should change this, agreed.

JonasToth updated this revision to Diff 119440.Oct 18 2017, 1:37 AM
JonasToth marked an inline comment as done.
  • address review comments
JonasToth marked an inline comment as done.Oct 18 2017, 1:37 AM
JonasToth updated this revision to Diff 119553.Oct 19 2017, 1:06 AM
  • Improve docs, grammar
JDevlieghere added inline comments.Oct 23 2017, 6:11 PM
80 ↗(On Diff #119553)

Maybe merge this with the function comment, no point in having both imho.

82 ↗(On Diff #119553)

LLVM style guide says no braces with single line if/else statements. No need for the else after a return. Alternatively you could use the ternary operator.

100 ↗(On Diff #119553)

No need for the else here. See previous comment.

146 ↗(On Diff #119553)


180 ↗(On Diff #119553)


183 ↗(On Diff #119553)

You can just use if because of the return.

186 ↗(On Diff #119553)

Drop the else

198 ↗(On Diff #119553)


JonasToth updated this revision to Diff 120251.Oct 25 2017, 7:40 AM
JonasToth marked 8 inline comments as done.
  • Merge 'master'
  • address review comments

simplified the if-else stuff

  • improved docs and comments
  • remove some newlines
  • use ternary operator instead of smallvec
  • fix rebasing issues in docs

@aaron.ballman could you take a (i think) finishing look on the check? Most issues should be resolved and i think its ready for the finishing line :)

JonasToth updated this revision to Diff 121134.Nov 1 2017, 8:37 AM
  • remove double whitespace
aaron.ballman added inline comments.Nov 15 2017, 10:39 AM
13 ↗(On Diff #121134)

Including <iostream> is forbidden by our coding standard. However, it doesn't appear to be used in the source file, either.

41 ↗(On Diff #121134)

either bitfield or integer condition -> either a bit-field or an integer condition

43 ↗(On Diff #121134)

since -> because

Also, drop the comma.

48 ↗(On Diff #121134)

binded -> bound

54 ↗(On Diff #121134)

Don't use doxygen comments here.

63 ↗(On Diff #121134)

Our usual preference is to make these methods static rather than put them in an anonymous namespace (types go into anonymous namespaces, however).

80 ↗(On Diff #121134)

This logic does not make sense to me -- the goal is to test for overflow, which can be done solely by looking at the value of Bits and the size of std::size_t. The current logic appears to return 1 if Bits == 0 and will only return the max value if the wraparound happens to precisely create the value 0 or 1. I'd replace with:

return Bits >= std::numeric_limits<std::size_t>::digits() ? std::numeric_limits<std::size_t>::max() : static_cast<size_t>(1) << Bits;
84 ↗(On Diff #121134)

How about: Get the number of possible values that can be switched on for the type T

91–92 ↗(On Diff #121134)

I don't think this comment adds value.

94 ↗(On Diff #121134)

No need for the ul suffix -- the type will undergo implicit conversion with or without the suffix.

94 ↗(On Diff #121134)

Why call toPow instead of just returning the value 1 directly?

102 ↗(On Diff #121134)

potential uncovered codepath -> potentially uncovered code path
branch -> statement

131 ↗(On Diff #121134)

No need to make this const (we only do that for reference and pointer types, not other local variable or parameter types). Here and elsewhere.

132 ↗(On Diff #121134)

statementt -> statement

138 ↗(On Diff #121134)

as if/else -> as an if/else

143–145 ↗(On Diff #121134)

Remove all commas.

161 ↗(On Diff #121134)

seperatly -> separately.

(period also)

176 ↗(On Diff #121134)

use if -> use an if

177 ↗(On Diff #121134)

potential uncovered codepath -> potentially uncovered code path
default case -> default label

21 ↗(On Diff #121134)

Drop the comma.

41–43 ↗(On Diff #121134)

How about:

This option can be configured to warn on missing 'else' branches in an 'if-else if' chain. The default is false because this option might be noisy on some code bases.
139 ↗(On Diff #121134)

Perhaps: Checks on switch and if-else if constructs that do not cover all possible code paths.

6 ↗(On Diff #121134)

Perhaps: This check discovers situations where code paths are not fully-covered.

7 ↗(On Diff #121134)

gets clearer with it -> will be more clear

16 ↗(On Diff #121134)

Perhaps: This warning can be noisy on some code bases, so it is disabled by default.

32 ↗(On Diff #121134)

, that -> which

62 ↗(On Diff #121134)

, that are not default -> other than a `default` label

63 ↗(On Diff #121134)

Otherwise should have a comma after it

I'd remove "common" and go with "with an `if` statement."

64 ↗(On Diff #121134)

catched -> caught

68 ↗(On Diff #121134)

if() -> an `if` statement:

86 ↗(On Diff #121134)

Completly -> A completely
warned -> diagnosed

drop the comma.

445 ↗(On Diff #121134)

For fun, can you add a switch over a value of type bool?

void f(bool b) {
  switch (b) {
  case true:

  switch (b) {

  switch (b) {
  case true:
  case false:
JonasToth updated this revision to Diff 123345.Nov 17 2017, 8:05 AM
JonasToth marked 18 inline comments as done.
  • rebase to master
  • address review comments from aaron
  • handle bools correctly
13 ↗(On Diff #121134)

Yes, forgot it from debugging.

91–92 ↗(On Diff #121134)

i dropped all of it. The function doc should be clear enough, is it?

94 ↗(On Diff #121134)


445 ↗(On Diff #121134)

There is already a warning in the frontend (-Wswitch-bool) that will complain about boolean conditions in switch.

I will implement the logic anyway. I think its more userfriendly to get this warning. One might have code style that allows switch for bools.

JonasToth marked 5 inline comments as done.Nov 17 2017, 8:08 AM
JonasToth updated this revision to Diff 123347.Nov 17 2017, 8:16 AM
JonasToth marked 12 inline comments as done.
  • address more comments, especially doc
JonasToth updated this revision to Diff 123349.Nov 17 2017, 8:25 AM
  • - fix ReleaseNotes
JonasToth marked an inline comment as done.Nov 17 2017, 8:25 AM
aaron.ballman accepted this revision.Nov 18 2017, 11:40 AM

Aside from some minor commenting nits, LGTM

168 ↗(On Diff #123349)

bitfield -> bit-field

41 ↗(On Diff #121134)

bitfield -> bit-field

91–92 ↗(On Diff #121134)

Yes, I think so.

This revision is now accepted and ready to land.Nov 18 2017, 11:40 AM
JonasToth marked 3 inline comments as done.
  • fix nits
This revision was automatically updated to reflect the committed changes.