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,51 @@ +//===--- 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 "clang/ASTMatchers/ASTMatchFinder.h" +#include + +namespace clang { +namespace tidy { +namespace hicpp { + +/// Find occasions 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: + void handleSwitchWithDefault(const SwitchStmt *Switch); + void handleSwitchWithoutDefault( + const SwitchStmt *Switch, + const ast_matchers::MatchFinder::MatchResult &Result); + /// 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. + 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,179 @@ +//===--- 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 + +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 + // 'switchStmt' options. + // 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 statements that have either a bitfield or an + // integer condition. The ordering in 'anyOf()' is important + // because the last condition is the most general. + anyOf(ignoringImpCasts(memberExpr(hasDeclaration( + fieldDecl(isBitField()).bind("bitfield")))), + hasDescendant(declRefExpr().bind("non-enum-condition"))), + // 'unless()' must be the last match here and must be bound, + // otherwise the matcher does not work correctly. + 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); + } +} + +static unsigned countCaseLabels(const SwitchStmt *Switch) { + unsigned CaseCount = 0; + + const SwitchCase *CurrentCase = Switch->getSwitchCaseList(); + while (CurrentCase) { + ++CaseCount; + CurrentCase = CurrentCase->getNextSwitchCase(); + } + + return CaseCount; +} +/// This function calculate 2 ** Bits and returns +/// numeric_limits::max() if an overflow occured. +static std::size_t twoPow(std::size_t Bits) { + return Bits >= std::numeric_limits::digits + ? std::numeric_limits::max() + : static_cast(1) << Bits; +} +/// Get the number of possible values that can be switched on for the type T. +/// +/// \return - 0 if bitcount could not be determined +/// - numeric_limits::max() when overflow appeared due to +/// more then 64 bits type size. +static std::size_t getNumberOfPossibleValues(QualType T, + const ASTContext &Context) { + // `isBooleanType` must come first because `bool` is an integral type as well + // and would not return 2 as result. + if (T->isBooleanType()) + return 2; + else if (T->isIntegralType(Context)) + return twoPow(Context.getTypeSize(T)); + else + return 1; +} + +void MultiwayPathsCoveredCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *ElseIfWithoutElse = + Result.Nodes.getNodeAs("else-if")) { + diag(ElseIfWithoutElse->getLocStart(), + "potentially uncovered codepath; add an ending else statement"); + return; + } + // Checks the sanity of 'switch' statements that actually do define + // a default branch but might be degenerated by having no or only one case. + if (const auto *SwitchWithDefault = + Result.Nodes.getNodeAs("switch-default")) { + handleSwitchWithDefault(SwitchWithDefault); + return; + } + // Checks all 'switch' statements that do not define a default label. + // Here the heavy lifting happens. + if (const auto *SwitchWithoutDefault = + Result.Nodes.getNodeAs("switch-no-default")) { + handleSwitchWithoutDefault(SwitchWithoutDefault, Result); + return; + } + // Warns for degenerated 'switch' statements that neither define a case nor + // a default label. + if (const auto *DegenerateSwitch = + Result.Nodes.getNodeAs("degenerate-switch")) { + diag(DegenerateSwitch->getLocStart(), "degenerated switch without labels"); + return; + } + llvm_unreachable("matched a case, that was not explicitly handled"); +} + +void MultiwayPathsCoveredCheck::handleSwitchWithDefault( + const SwitchStmt *Switch) { + unsigned CaseCount = countCaseLabels(Switch); + assert(CaseCount > 0 && "Switch statement with supposedly one default " + "branch did not contain any case labels"); + if (CaseCount == 1 || CaseCount == 2) + diag(Switch->getLocStart(), + CaseCount == 1 + ? "degenerated switch with default label only" + : "switch could be better written as an if/else statement"); +} + +void MultiwayPathsCoveredCheck::handleSwitchWithoutDefault( + const SwitchStmt *Switch, const MatchFinder::MatchResult &Result) { + // 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' statements without a 'default' branch. + assert(!Result.Nodes.getNodeAs("enum-condition") && + "switch over enum is handled by warnings already, explicitly ignoring " + "them"); + assert(!Result.Nodes.getNodeAs("switch-default") && + "switch stmts with default branch do cover all paths, explicitly " + "ignoring them"); + // Determine the number of case labels. Since 'default' is not present + // and duplicating case labels is not allowed this number represents + // the number of codepaths. It can be directly compared to 'MaxPathsPossible' + // to see if some cases are missing. + unsigned CaseCount = countCaseLabels(Switch); + // CaseCount == 0 is caught in DegenerateSwitch. Necessary because the + // matcher used for here does not match on degenerate 'switch'. + assert(CaseCount > 0 && "Switch statement without any case found. This case " + "should be excluded by the matcher and is handled " + "separatly."); + std::size_t MaxPathsPossible = [&]() { + if (const auto *GeneralCondition = + Result.Nodes.getNodeAs("non-enum-condition")) + return getNumberOfPossibleValues(GeneralCondition->getType(), + *Result.Context); + if (const auto *BitfieldDecl = + Result.Nodes.getNodeAs("bitfield")) + return twoPow(BitfieldDecl->getBitWidthValue(*Result.Context)); + llvm_unreachable("either bitfield or non-enum must be condition"); + }(); + + // FIXME: Transform the 'switch' into an 'if' for CaseCount == 1. + if (CaseCount < MaxPathsPossible) + diag(Switch->getLocStart(), + CaseCount == 1 ? "switch with only one case; use an if statement" + : "potential uncovered code path; add a default label"); +} +} // namespace hicpp +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -158,6 +158,11 @@ 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 ``switch`` and ``if`` - ``else if`` constructs that do not cover all possible code paths. + - 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 discovers situations where code paths are not fully-covered. +It furthermore suggests using ``if`` instead of ``switch`` if the code will be more clear. +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. +This warning can be noisy on some code bases, so it is disabled by default. + +.. 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 which 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 other than a `default` label. +Otherwise, the ``switch`` could be better expressed with an ``if`` statement. +Degenerated ``switch`` statements without any labels are caught 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++ + + // A completly degenerated switch will be diagnosed. + 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 @@ -81,6 +81,7 @@ hicpp-invalid-access-moved (redirects to misc-use-after-move) hicpp-member-init (redirects to cppcoreguidelines-pro-type-member-init) hicpp-move-const-arg (redirects to misc-move-const-arg) + 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,57 @@ +// 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: potentially uncovered codepath; add an ending else statement + return; + } + + // Could be considered as false positive because all paths are covered logically. + // I still think this is valid since the possibility of a final 'everything else' + // codepath is expected from if-else if. + if (i > 0) { + return; + } else if (i <= 0) { + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: potentially uncovered codepath; add an ending else statement + 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: potentially uncovered codepath; add an ending else statement + 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,467 @@ +// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t + +enum OS { Mac, + Windows, + Linux }; + +struct Bitfields { + unsigned UInt : 3; + int SInt : 1; +}; + +int return_integer() { return 42; } + +void bad_switch(int i) { + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement + case 0: + break; + } + // No default in this switch + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label + case 0: + break; + case 1: + break; + case 2: + break; + } + + // degenerate, maybe even warning + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels + } + + switch (int j = return_integer()) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label + case 0: + case 1: + case 2: + break; + } + + // Degenerated, only default case. + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only + default: + break; + } + + // Degenerated, only one case label and default case -> Better as if-stmt. + switch (i) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as an if/else statement + case 0: + break; + default: + break; + } + + unsigned long long BigNumber = 0; + switch (BigNumber) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label + case 0: + case 1: + break; + } + + const int &IntRef = i; + switch (IntRef) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label + case 0: + case 1: + break; + } + + char C = 'A'; + switch (C) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label + case 'A': + break; + case 'B': + break; + } + + Bitfields Bf; + // UInt has 3 bits size. + switch (Bf.UInt) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label + case 0: + case 1: + break; + } + // All paths explicitly covered. + switch (Bf.UInt) { + case 0: + case 1: + case 2: + case 3: + case 4: + case 5: + case 6: + case 7: + break; + } + // SInt has 1 bit size, so this is somewhat degenerated. + switch (Bf.SInt) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement + case 0: + break; + } + // All paths explicitly covered. + switch (Bf.SInt) { + case 0: + case 1: + break; + } + + bool Flag = false; + switch (Flag) { + // CHECK-MESSAGES:[[@LINE-1]]:3: warning: switch with only one case; use an if statement + case true: + break; + } + + switch (Flag) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only + default: + break; + } + + // This `switch` will create a frontend warning from '-Wswitch-bool' but is + // ok for this check. + switch (Flag) { + case true: + break; + case false: + break; + } +} + +void unproblematic_switch(unsigned char c) { + switch (c) { + 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 case 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) { + } +} + +/// All of these cases will not emit a warning per default, but with explicit activation. +/// Covered in extra test file. +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 */ + } +}