Differential D37808
[clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches Authored by JonasToth on Sep 13 2017, 6:51 AM.
Details
This check searches for missing else branches in if-else if-chains and It is very similar to -Wswitch, but concentrates on integers only, since enums are The option to warn for missing else branches is deactivated by default, since it is Running it on LLVM: just for llvm/lib/Analysis/ScalarEvolution.cpp, the else-path checker is very noisy!
Diff Detail
Event TimelineComment Actions I added a clarifying comment for some outcommented code still in the patch. I would like to have a bit of discussion on it.
Comment Actions Ok. But it will introduce new warnings to llvm codebase itself. I prepare some example output i found right now. Comment Actions If number of them will not be huge, it'll be worth to fix before extended -Wswitch will be committed. Comment Actions Strictly speaking, the coding standard (which should be enforced by the patch) requires strict ISO C++11, therefore this extension is not considered directly. Comment Actions
It does: test.cpp:3:12: warning: use of GNU case range extension [-Wgnu-case-range]
case 1 ... 3:
^
Comment Actions Where would be the correct place to further discuss extending -Wswitch ? Should i ask that on the mailing list? Comment Actions 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) 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. void maybefalse(int i) {
if (i > 0) {
return;
} else if (i <= 0) {
return;
}Comment Actions 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. Comment Actions ping: I don't understand the lack of feedback. This check should not be a frontend warning,
Comment Actions I very much like this check. I only have a few minor comments, but maybe this encourages others to have a look too!
Comment Actions major codechange
Comment Actions 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.
Comment Actions
Comment Actions @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 :)
Comment Actions
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||