This is an archive of the discontinued LLVM Phabricator instance.

[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!

Diff Detail

Repository
rL LLVM

Event Timeline

JonasToth created this revision.Sep 13 2017, 6:51 AM
JonasToth retitled this revision from [clang-tidy] Add new hicpp-multiway-paths-covered check, for missing branches to [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches.Sep 13 2017, 6:52 AM
JonasToth set the repository for this revision to rL LLVM.
JonasToth added a project: Restricted Project.
JonasToth added a subscriber: cfe-commits.

I added a clarifying comment for some outcommented code still in the patch. I would like to have a bit of discussion on it.
Maybe some parts of the check could be warning-area, i just implemented it here to have it on one place.

clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
47 ↗(On Diff #115034)

s/noise/noisy/

68–78 ↗(On Diff #115034)

I originally tried to implement some logic, that will realize when all paths are covered explicitly.
It didn't work and i think it is irrelevant anyway, since manually covering all paths for integertypes is IMHO unrealistic.

Iam willing to remove the code i currently commented out, just wanted it to be there if there is explicit need for a 'is everything covered'-check.

docs/clang-tidy/checks/list.rst
41 ↗(On Diff #115034)

remove

test/clang-tidy/hicpp-multiway-paths-covered.cpp
7 ↗(On Diff #115034)

explicitly test, that the warnings for else do not occur here.

JonasToth updated this revision to Diff 115037.Sep 13 2017, 7:06 AM
JonasToth marked 3 inline comments as done.
  • fix the first issues i found

fixed first stuff, sorry for noise.

I think will be good idea to extend -Wswitch diagnostics.

I think will be good idea to extend -Wswitch diagnostics.

Ok. But it will introduce new warnings to llvm codebase itself. I prepare some example output i found right now.

JonasToth edited the summary of this revision. (Show Details)Sep 13 2017, 7:26 AM

I think will be good idea to extend -Wswitch diagnostics.

Ok. But it will introduce new warnings to llvm codebase itself. I prepare some example output i found right now.

If number of them will not be huge, it'll be worth to fix before extended -Wswitch will be committed.

What about GNU extension case 1 ... 3: ?

What about GNU extension case 1 ... 3: ?

Strictly speaking, the coding standard (which should be enforced by the patch) requires strict ISO C++11, therefore this extension is not considered directly.
Does clang support this extension? When this would be a warning, then that case should be added as well.

What about GNU extension case 1 ... 3: ?

Strictly speaking, the coding standard (which should be enforced by the patch) requires strict ISO C++11, therefore this extension is not considered directly.

Does clang support this extension?

It does:

test.cpp:3:12: warning: use of GNU case range extension [-Wgnu-case-range]
    case 1 ... 3:
           ^

When this would be a warning, then that case should be added as well.

I think will be good idea to extend -Wswitch diagnostics.

Ok. But it will introduce new warnings to llvm codebase itself. I prepare some example output i found right now.

If number of them will not be huge, it'll be worth to fix before extended -Wswitch will be committed.

Where would be the correct place to further discuss extending -Wswitch ? Should i ask that on the mailing list?

I think will be good idea to extend -Wswitch diagnostics.

Ok. But it will introduce new warnings to llvm codebase itself. I prepare some example output i found right now.

If number of them will not be huge, it'll be worth to fix before extended -Wswitch will be committed.

The other way around, all new warnings must to be fixed first, before committing the new diagnostic itself.

I might agree that the part of this check that is handling switch() might indeed be better off extending -Wswitch (as several new flags -Wswitch-missing-default, -Wswitch-empty, -Wswitch-prefer-if, or something like that)
But then a much greater thought shall be put into ensuring quality of the new diagnostic.

Oh, and i'd say the WarnOnMissingElse should *not* be moved to clang diagnostic. At least right now it will result in many false-positives.
E.g. what would it say about

void maybefalse(int i) {
  if (i > 0) {
    return;
  } else if (i <= 0) {
    return;
  }
JonasToth added a comment.EditedSep 16 2017, 5:38 AM

I on my part think that both switch and else should stay here for now. Running it over llvm gave really a lot of warnings, and that a rather good codebase.

The check does not recognize logic, your example would be warned against. I don't see a way to implement something like that here, that would require condition solving and so on.

In general, this check is more about defensive programming and therefore clang-tidy a good place.

ping. is there something obviously wrong with this check?

JonasToth updated this revision to Diff 118911.Oct 13 2017, 8:02 AM

ping: I don't understand the lack of feedback. This check should not be a frontend warning,
since it warns for perfectly valid code. It is supposed to give stronger guarantees
for developers requiring this.

  • rebased to master

I very much like this check. I only have a few minor comments, but maybe this encourages others to have a look too!

clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
85 ↗(On Diff #118911)

I'm not a big fan of the 'found', can we just omit it? The same goes for the other diags.

96 ↗(On Diff #118911)

Why not a switch?

152 ↗(On Diff #118911)

I'm aware that the message and fixme are different, but since the structure is so similar to the handling of the other switch case, I wonder if there is any chance we could extract the common parts?

JonasToth added inline comments.Oct 13 2017, 8:46 AM
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
85 ↗(On Diff #118911)

Agree. The messages are better without 'found'.

96 ↗(On Diff #118911)

I intent to check if all cases are explicitly covered.

In the testcases there is one switch with all numbers explicitly written, meaning there is no need to add a default branch.

This would allow further

else if (CaseCount == MaximumPossibleCases) { /* No warning */ }

path which is not modable with switch.

152 ↗(On Diff #118911)

I try to get something shorter.
Maybe

if(CaseCount == 1 && MatchedSwitch) {}
else if(CaseCount == 1 && MatchedElseIf) {}

?

JDevlieghere added inline comments.Oct 13 2017, 10:58 AM
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
96 ↗(On Diff #118911)

Sounds good

152 ↗(On Diff #118911)

Wouldn't it be easier to have a function and pass as arguments the stuff that's different? Both are SwitchStmts so if you pass that + the diagnostic message you should be able to share the other logic.

JonasToth updated this revision to Diff 118951.EditedOct 13 2017, 12:00 PM
JonasToth marked 2 inline comments as done.

major codechange

  • simplify structure of checking and extract functions
  • handle bitfields as well
JonasToth marked 6 inline comments as done.Oct 13 2017, 12:02 PM
JonasToth added inline comments.Oct 13 2017, 12:02 PM
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
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
}
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 ↗(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.
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
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
clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
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)

Braces

180 ↗(On Diff #119553)

Braces

183 ↗(On Diff #119553)

You can just use if because of the return.

186 ↗(On Diff #119553)

Drop the else

198 ↗(On Diff #119553)

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

clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
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.
docs/ReleaseNotes.rst
139 ↗(On Diff #121134)

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
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.

test/clang-tidy/hicpp-multiway-paths-covered.cpp
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) {
  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
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)

True!

test/clang-tidy/hicpp-multiway-paths-covered.cpp
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

clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
41 ↗(On Diff #121134)

bitfield -> bit-field

91–92 ↗(On Diff #121134)

Yes, I think so.

168 ↗(On Diff #123349)

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.