Page MenuHomePhabricator

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

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

Details

Summary

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!

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
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
97

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

153

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
}
llvm_unreachable("reason");

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
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
76

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

98

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

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

195

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.
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
98

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
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
81

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

83

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.

101

No need for the else here. See previous comment.

147

Braces

181

Braces

184

You can just use if because of the return.

187

Drop the else

199

Braces

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
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
14

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

42

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

44

since -> because

Also, drop the comma.

49

binded -> bound

55

Don't use doxygen comments here.

64

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

81

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;
85

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

92–93

I don't think this comment adds value.

95

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

95

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

103

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

132

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.

133

statementt -> statement

139

as if/else -> as an if/else

144–146

Remove all commas.

162

seperatly -> separately.

(period also)

177

use if -> use an if

178

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

clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
22

Drop the comma.

42–44

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.
docs/ReleaseNotes.rst
131

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

docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
7

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

8

gets clearer with it -> will be more clear

17

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

33

, that -> which

63

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

64

Otherwise should have a comma after it

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

65

catched -> caught

69

if() -> an `if` statement:

87

Completly -> A completely
warned -> diagnosed

drop the comma.

test/clang-tidy/hicpp-multiway-paths-covered.cpp
446

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

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

  switch (b) {
  default:
  }

  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
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
14

Yes, forgot it from debugging.

92–93

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

95

True!

test/clang-tidy/hicpp-multiway-paths-covered.cpp
446

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

clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
42

bitfield -> bit-field

92–93

Yes, I think so.

169

bitfield -> bit-field

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.