Index: clang-tidy/hicpp/CMakeLists.txt =================================================================== --- clang-tidy/hicpp/CMakeLists.txt +++ clang-tidy/hicpp/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyHICPPModule ExceptionBaseclassCheck.cpp + MultiwayPathsCoveredCheck.cpp NoAssemblerCheck.cpp HICPPTidyModule.cpp SignedBitwiseCheck.cpp Index: clang-tidy/hicpp/HICPPTidyModule.cpp =================================================================== --- clang-tidy/hicpp/HICPPTidyModule.cpp +++ clang-tidy/hicpp/HICPPTidyModule.cpp @@ -35,6 +35,7 @@ #include "../readability/FunctionSizeCheck.h" #include "../readability/IdentifierNamingCheck.h" #include "ExceptionBaseclassCheck.h" +#include "MultiwayPathsCoveredCheck.h" #include "NoAssemblerCheck.h" #include "SignedBitwiseCheck.h" @@ -53,6 +54,8 @@ "hicpp-exception-baseclass"); CheckFactories.registerCheck( "hicpp-signed-bitwise"); + CheckFactories.registerCheck( + "hicpp-multiway-paths-covered"); CheckFactories.registerCheck( "hicpp-explicit-conversions"); CheckFactories.registerCheck( Index: clang-tidy/hicpp/MultiwayPathsCoveredCheck.h =================================================================== --- /dev/null +++ clang-tidy/hicpp/MultiwayPathsCoveredCheck.h @@ -0,0 +1,46 @@ +//===--- MultiwayPathsCoveredCheck.h - clang-tidy----------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAY_PATHS_COVERED_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAY_PATHS_COVERED_H + +#include "../ClangTidy.h" +#include + +namespace clang { +namespace tidy { +namespace hicpp { + +/// Find occasion, where not all codepaths are explicitly covered in code. +/// This includes 'switch' without a 'default'-branch and if-else-if-chains +/// without a final 'else'-branch. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-multiway-paths-covered.html +class MultiwayPathsCoveredCheck : public ClangTidyCheck { +public: + MultiwayPathsCoveredCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + WarnOnMissingElse(Options.get("WarnOnMissingElse", 0)) {} + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + /// This option configures if there is a warning on missing 'else'-branches + /// in 'if-else-if'-chains. The default is false, since this option might be + /// very noisy on particular codebases. + const bool WarnOnMissingElse; +}; + +} // namespace hicpp +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAY_PATHS_COVERED_H Index: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp @@ -0,0 +1,188 @@ +//===--- MultiwayPathsCoveredCheck.cpp - clang-tidy------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "MultiwayPathsCoveredCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace hicpp { + +void MultiwayPathsCoveredCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "WarnOnMissingElse", WarnOnMissingElse); +} + +void MultiwayPathsCoveredCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + stmt(allOf(anyOf(switchStmt(forEachSwitchCase(defaultStmt())) + .bind("switch-default"), + switchStmt(unless(hasDescendant(switchCase()))) + .bind("degenerate-switch"), + // This matcher must be the last one of the three, + // otherwise the cases "switch-default" and + // "degenerate-switch" are not found correctly. + switchStmt(forEachSwitchCase(unless(defaultStmt()))) + .bind("switch-no-default")), + switchStmt(hasCondition(allOf( + // Match on switch stmt that have integer conditions + // (everything, that is not an enum). Here again, the + // ordering is important. Otherwise the matching fails. + hasDescendant(declRefExpr().bind("non-enum-condition")), + unless(hasDescendant(declRefExpr(hasType(enumType())) + .bind("enum-condition")))))))), + this); + + /// This option is noisy, therefore matching is configurable. + if (WarnOnMissingElse) { + Finder->addMatcher( + ifStmt(allOf(hasParent(ifStmt()), unless(hasElse(anything())))) + .bind("else-if"), + this); + } +} + +namespace { +unsigned countCaseLabels(const SwitchStmt *Switch) { + unsigned CaseCount = 0; + + const SwitchCase *CurrentCase = Switch->getSwitchCaseList(); + while (CurrentCase) { + ++CaseCount; + CurrentCase = CurrentCase->getNextSwitchCase(); + } + + return CaseCount; +} +#if 0 +/// Get the number of different values for the Type T, that is switched on. +std::size_t getNumberOfPossibleValues(const Type *T, + const ASTContext &Context) { + // This Assertion fails in clang and llvm, when matching on enum types. + assert(T->isIntegralType(Context) && + "expecting integral type for discrete set of values"); + + return std::numeric_limits::max(); +} +#endif +} // namespace + +void MultiwayPathsCoveredCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *ElseIfWithoutElse = + Result.Nodes.getNodeAs("else-if")) { + diag(ElseIfWithoutElse->getLocStart(), + "potential uncovered codepath found; add an ending else branch"); + return; + } + + if (const auto *SwitchWithDefault = + Result.Nodes.getNodeAs("switch-default")) { + const unsigned CaseCount = countCaseLabels(SwitchWithDefault); + assert(CaseCount > 0 && "Switch stmt with supposedly one default branch " + "did not contain case labels"); + + // Only the default branch (we explicitly matched for default!) exists. + if (CaseCount == 1) { + diag(SwitchWithDefault->getLocStart(), + "degenerated switch stmt found; only the default branch exists"); + // FIXME: The condition and everything could be removed, since default + // will be always executed. + } + // One case label and a default branch exists. + else if (CaseCount == 2) { + diag(SwitchWithDefault->getLocStart(), + "switch case could be better written with single if stmt"); + // FIXME: Rewrite the switch into an if-stmt. + // The case lands in + // if ((condition) == caseLabel) { stmt... } + // else { default_stmts } + } + // Multiply cases and a default branch exist, therefore everything is + // alright with the switch stmt. + else { + return; + } + return; + } + + if (const auto *SwitchWithoutDefault = + Result.Nodes.getNodeAs("switch-no-default")) { + // The matcher only works, because some nodes are explicitly matched and + // bound, but ignored. This is necessary, to build the excluding logic for + // enums and switch stmts without a default branch. + assert(!Result.Nodes.getNodeAs("enum-condition") && + "switch over enum is handled by warnings already, explicitly ignore " + "them"); + assert(!Result.Nodes.getNodeAs("switch-default") && + "switch stmts with default branch do cover all paths, explicitly " + "ignore them"); + + const auto *IntegerCondition = + Result.Nodes.getNodeAs("non-enum-condition"); + assert(IntegerCondition && "Did not find pure integer condition, but did " + "match explicitly only for integers"); +#if 0 + // Determining the correct number of values currently not working. + const std::size_t MaxPathsPossible = getNumberOfPossibleValues( + IntegerCondition->getType().getCanonicalType().split().Ty, + *Result.Context); +#endif + + // Check if all paths were covered explicitly. + const unsigned CaseCount = countCaseLabels(SwitchWithoutDefault); + + // CaseCount == 0 is caught in DegenerateSwitch. Necessary because the + // matcher used for here does not match on degenerate stmt + assert(CaseCount > 0 && "Switch stmt without any case found. This case " + "should be excluded by the matcher and is handled " + "seperatly"); + + // Should be written as an IfStmt. + if (CaseCount == 1) { + diag(SwitchWithoutDefault->getLocStart(), "switch stmt with only one " + "case found; this can be " + "better written in an if-stmt"); + // FIXME: Automatically transform the switch into an if. + } + // Missed some value explicity, therefore add a default branch. + else if (CaseCount >= 2 /* && CaseCount < MaxPathsPossible*/) { + diag(SwitchWithoutDefault->getLocStart(), + "potential uncovered codepath found; add a default branch"); + // FIXME: Automatically add 'default: break;' after the last case. + } + // All paths were explicitly covered, therefore no default branch is + // necessary. + else { + return; + } + + return; + } + + if (const auto *DegenerateSwitch = + Result.Nodes.getNodeAs("degenerate-switch")) { + diag(DegenerateSwitch->getLocStart(), "degenerate switch statement without " + "any cases found; add cases and a " + "default branch"); + // FIXME: Removing would be possible? But i think it should not be removed, + // since it is most likely an error. + return; + } + + llvm_unreachable("matched a case, that was not explicitly handled"); +} + +} // namespace hicpp +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -125,6 +125,12 @@ Ensures that all exception will be instances of ``std::exception`` and classes that are derived from it. +- New `hicpp-multiway-paths-covered + `_ check + + Checks on various possible constellations, where ``switch`` and ``if-else if`` statements + do not cover all possible codepaths. + - New `hicpp-signed-bitwise `_ check Index: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst @@ -0,0 +1,95 @@ +.. title:: clang-tidy - hicpp-multiway-paths-covered + +hicpp-multiway-paths-covered +============================ + +This check catches multiple occasion, where not all possible code paths are covered. +The `rule 6.1.2 `_ +and `rule 6.1.4 `_ +of the High Integrity C++ Coding Standard are enforced. + +``if-else if`` chains that miss a final ``else`` branch might lead to unexpected +program execution and be the result of a logical error. +If the missing ``else`` branch is intended, you can leave it empty with a clarifying +comment. +Since this warning might be very noise on many codebases it is configurable and by +default deactivated. + +.. code-block:: c++ + + void f1() { + int i = determineTheNumber(); + + if(i > 0) { + // Some Calculation + } else if (i < 0) { + // Precondition violated or something else. + } + // ... + } + +Similar arguments hold for ``switch`` statements, that do not cover all possible code paths. + +.. code-block:: c++ + + // The missing default branch might be a logical error. It can be kept empty + // if there is nothing to do, making it explicit. + void f2(int i) { + switch (i) { + case 0: // something + break; + case 1: // something else + break; + } + // All other numbers? + } + + // Violates this rule as well, but already emits a compiler warning (-Wswitch). + enum Color { Red, Green, Blue, Yellow }; + void f3(enum Color c) { + switch (c) { + case Red: // We can't drive for now. + break; + case Green: // We are allowed to drive. + break; + } + // Other cases missing + } + + +The `rule 6.1.4 `_ +requires every ``switch`` statement to have at least two ``case`` labels, that are not default. +Otherwise the ``switch`` could be better expressed with a common ``if`` statement. +Completly degenerated ``switch`` statements without any labels are catched as well. + +.. code-block:: c++ + + // Degenerated switch, that could be better written as if() + int i = 42; + switch(i) { + case 1: // do something here + default: // do somethe else here + } + + // Should rather be the following: + if (i == 1) { + // do something here + } + else { + // do something here + } + + +.. code-block:: c++ + + // Completly degenerated switch, will be warned. + int i = 42; + switch(i) {} + + +Options +------- + +.. option:: WarnOnMissingElse + + Activates warning for missing``else`` branches. Default is `0`. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -78,6 +78,7 @@ hicpp-invalid-access-moved (redirects to misc-use-after-move) hicpp-move-const-arg (redirects to misc-move-const-arg) hicpp-member-init (redirects to cppcoreguidelines-pro-type-member-init) + hicpp-multiway-paths-covered hicpp-named-parameter (redirects to readability-named-parameter) hicpp-new-delete-operators (redirects to misc-new-delete-overloads) hicpp-no-array-decay (redirects to cppcoreguidelines-pro-bounds-array-to-pointer-decay) Index: test/clang-tidy/hicpp-multiway-paths-covered-else.cpp =================================================================== --- /dev/null +++ test/clang-tidy/hicpp-multiway-paths-covered-else.cpp @@ -0,0 +1,47 @@ +// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: hicpp-multiway-paths-covered.WarnOnMissingElse, value: 1}]}'\ +// RUN: -- + +enum OS { Mac, + Windows, + Linux }; + +void problematic_if(int i, enum OS os) { + if (i > 0) { + return; + } else if (i < 0) { + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: potential uncovered codepath found; add an ending else branch + return; + } + + // Test if nesting of if-else chains does get caught as well. + if (os == Mac) { + return; + } else if (os == Linux) { + // These checks are kind of degenerated, but the check will not try to solve + // if logically all paths are covered, which is more the area of the static analyzer. + if (true) { + return; + } else if (false) { + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: potential uncovered codepath found; add an ending else branch + return; + } + return; + } else { + /* unreachable */ + if (true) // check if the parent would match here as well + return; + // No warning for simple if statements, since it is common to just test one condition + // and ignore the opposite. + } + + // Ok, because all paths are covered + if (i > 0) { + return; + } else if (i < 0) { + return; + } else { + /* error, maybe precondition failed */ + } +} Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp =================================================================== --- /dev/null +++ test/clang-tidy/hicpp-multiway-paths-covered.cpp @@ -0,0 +1,406 @@ +// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t + +enum OS { Mac, + Windows, + Linux }; + +/// All of these cases will not emit a warning per default, but with explicit activation. +void problematic_if(int i, enum OS os) { + if (i > 0) { + return; + } else if (i < 0) { + return; + } + + if (os == Mac) { + return; + } else if (os == Linux) { + if (true) { + return; + } else if (false) { + return; + } + return; + } else { + /* unreachable */ + if (true) // check if the parent would match here as well + return; + } + + // Ok, because all paths are covered + if (i > 0) { + return; + } else if (i < 0) { + return; + } else { + /* error, maybe precondition failed */ + } +} + +int return_integer() { return 42; } + +void problematic_switch(int i) { + // No default in this switch + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch + case 0: + break; + case 1: + break; + case 2: + break; + } + + // degenerate, maybe even warning + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerate switch statement without any cases found; add cases and a default branch + } + + switch (int j = return_integer()) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch + case 0: + case 1: + case 2: + break; + } + + // Degenerated, only default branch. + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch stmt found; only the default branch exists + default: + break; + } + + // Degenerated, only one case label and default branch -> Better as if-stmt. + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch case could be better written with single if stmt + case 0: + break; + default: + break; + } + + unsigned long long BigNumber = 0; + switch (BigNumber) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch + case 0: + case 1: + break; + } + + const int &IntRef = i; + switch (IntRef) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch + case 0: + case 1: + break; + } + + char C = 'A'; + switch (C) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch + case 'A': + break; + case 'B': + break; + } +} + +void unproblematic_switch(unsigned char c) { + // The switch stmt covers all paths explicitly. + // FIXME: False positive, determine the count of different possible values correctly. + // On the other hand, char is not defined to be one byte big, so there could be + // architectural differences, which would make the 'default:' case more portable. + switch (c) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered codepath found; add a default branch + case 0: + case 1: + case 2: + case 3: + case 4: + case 5: + case 6: + case 7: + case 8: + case 9: + case 10: + case 11: + case 12: + case 13: + case 14: + case 15: + case 16: + case 17: + case 18: + case 19: + case 20: + case 21: + case 22: + case 23: + case 24: + case 25: + case 26: + case 27: + case 28: + case 29: + case 30: + case 31: + case 32: + case 33: + case 34: + case 35: + case 36: + case 37: + case 38: + case 39: + case 40: + case 41: + case 42: + case 43: + case 44: + case 45: + case 46: + case 47: + case 48: + case 49: + case 50: + case 51: + case 52: + case 53: + case 54: + case 55: + case 56: + case 57: + case 58: + case 59: + case 60: + case 61: + case 62: + case 63: + case 64: + case 65: + case 66: + case 67: + case 68: + case 69: + case 70: + case 71: + case 72: + case 73: + case 74: + case 75: + case 76: + case 77: + case 78: + case 79: + case 80: + case 81: + case 82: + case 83: + case 84: + case 85: + case 86: + case 87: + case 88: + case 89: + case 90: + case 91: + case 92: + case 93: + case 94: + case 95: + case 96: + case 97: + case 98: + case 99: + case 100: + case 101: + case 102: + case 103: + case 104: + case 105: + case 106: + case 107: + case 108: + case 109: + case 110: + case 111: + case 112: + case 113: + case 114: + case 115: + case 116: + case 117: + case 118: + case 119: + case 120: + case 121: + case 122: + case 123: + case 124: + case 125: + case 126: + case 127: + case 128: + case 129: + case 130: + case 131: + case 132: + case 133: + case 134: + case 135: + case 136: + case 137: + case 138: + case 139: + case 140: + case 141: + case 142: + case 143: + case 144: + case 145: + case 146: + case 147: + case 148: + case 149: + case 150: + case 151: + case 152: + case 153: + case 154: + case 155: + case 156: + case 157: + case 158: + case 159: + case 160: + case 161: + case 162: + case 163: + case 164: + case 165: + case 166: + case 167: + case 168: + case 169: + case 170: + case 171: + case 172: + case 173: + case 174: + case 175: + case 176: + case 177: + case 178: + case 179: + case 180: + case 181: + case 182: + case 183: + case 184: + case 185: + case 186: + case 187: + case 188: + case 189: + case 190: + case 191: + case 192: + case 193: + case 194: + case 195: + case 196: + case 197: + case 198: + case 199: + case 200: + case 201: + case 202: + case 203: + case 204: + case 205: + case 206: + case 207: + case 208: + case 209: + case 210: + case 211: + case 212: + case 213: + case 214: + case 215: + case 216: + case 217: + case 218: + case 219: + case 220: + case 221: + case 222: + case 223: + case 224: + case 225: + case 226: + case 227: + case 228: + case 229: + case 230: + case 231: + case 232: + case 233: + case 234: + case 235: + case 236: + case 237: + case 238: + case 239: + case 240: + case 241: + case 242: + case 243: + case 244: + case 245: + case 246: + case 247: + case 248: + case 249: + case 250: + case 251: + case 252: + case 253: + case 254: + case 255: + break; + } + + // Some paths are covered by the switch and a default branch is present. + switch (c) { + case 1: + case 2: + case 3: + default: + break; + } +} + +OS return_enumerator() { + return Linux; +} + +// Enumpaths are already covered by a warning, this is just to ensure, that there is +// no interference or false positives. +// -Wswitch warns about uncovered enum paths and each here described case is already +// covered. +void switch_enums(OS os) { + switch (os) { + case Linux: + break; + } + + switch (OS another_os = return_enumerator()) { + case Linux: + break; + } + + switch (os) { + } +}