[clang-tools-extra] r318600 - [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches
Mike Edwards via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 20 09:24:35 PST 2017
Hi,
We are seeing a bot failure with this commit. Please see the bot page here:
http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/5470/consoleFull#17462642768254eaf0-7326-4999-85b0-388101f2d404
Please let me know if you will be able to provide a patch for this in the
next couple of hours, otherwise I will need to revert your commit. Thank
you for your assistance in keeping our bots green.
Respectfully,
Mike Edwards
On Sat, Nov 18, 2017 at 11:48 AM, Jonas Toth via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Author: jonastoth
> Date: Sat Nov 18 11:48:33 2017
> New Revision: 318600
>
> URL: http://llvm.org/viewvc/llvm-project?rev=318600&view=rev
> Log:
> [clang-tidy] Add new hicpp-multiway-paths-covered check for missing
> branches
>
> 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:
> {F5354858} for default configuration
> {F5354866} just for llvm/lib/Analysis/ScalarEvolution.cpp, the else-path
> checker is very noisy!
>
> Reviewers: alexfh, aaron.ballman, hokein
>
> Reviewed By: aaron.ballman
>
> Subscribers: lebedev.ri, Eugene.Zelenko, cfe-commits, mgorny,
> JDevlieghere, xazax.hun
>
> Tags: #clang-tools-extra
>
> Differential Revision: https://reviews.llvm.org/D37808
>
>
> Added:
> clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
> clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
> clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-
> multiway-paths-covered.rst
> clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-
> paths-covered-else.cpp
> clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-
> paths-covered.cpp
> Modified:
> clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt
> clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
> clang-tools-extra/trunk/docs/ReleaseNotes.rst
> clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
>
> Modified: clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clang-tidy/hicpp/CMakeLists.txt?rev=318600&r1=
> 318599&r2=318600&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt (original)
> +++ clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt Sat Nov 18
> 11:48:33 2017
> @@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
>
> add_clang_library(clangTidyHICPPModule
> ExceptionBaseclassCheck.cpp
> + MultiwayPathsCoveredCheck.cpp
> NoAssemblerCheck.cpp
> HICPPTidyModule.cpp
> SignedBitwiseCheck.cpp
>
> Modified: clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clang-tidy/hicpp/HICPPTidyModule.cpp?rev=318600&r1=318599&r2=318600&
> view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
> (original)
> +++ clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp Sat Nov
> 18 11:48:33 2017
> @@ -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 @@ public:
> "hicpp-exception-baseclass");
> CheckFactories.registerCheck<SignedBitwiseCheck>(
> "hicpp-signed-bitwise");
> + CheckFactories.registerCheck<MultiwayPathsCoveredCheck>(
> + "hicpp-multiway-paths-covered");
> CheckFactories.registerCheck<google::ExplicitConstructorCheck>(
> "hicpp-explicit-conversions");
> CheckFactories.registerCheck<readability::FunctionSizeCheck>(
>
> Added: clang-tools-extra/trunk/clang-tidy/hicpp/
> MultiwayPathsCoveredCheck.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp?rev=318600&view=auto
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
> (added)
> +++ clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
> Sat Nov 18 11:48:33 2017
> @@ -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 <limits>
> +
> +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 bit-field
> 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<std::size_t>::max() if an overflow occured.
> +static std::size_t twoPow(std::size_t Bits) {
> + return Bits >= std::numeric_limits<std::size_t>::digits
> + ? std::numeric_limits<std::size_t>::max()
> + : static_cast<size_t>(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<std::size_t>::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<IfStmt>("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<SwitchStmt>("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<SwitchStmt>("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<SwitchStmt>("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<DeclRefExpr>("enum-condition") &&
> + "switch over enum is handled by warnings already, explicitly
> ignoring "
> + "them");
> + assert(!Result.Nodes.getNodeAs<SwitchStmt>("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<DeclRefExpr>("non-enum-condition"))
> + return getNumberOfPossibleValues(GeneralCondition->getType(),
> + *Result.Context);
> + if (const auto *BitfieldDecl =
> + Result.Nodes.getNodeAs<FieldDecl>("bitfield"))
> + return twoPow(BitfieldDecl->getBitWidthValue(*Result.Context));
> + llvm_unreachable("either bit-field 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
>
> Added: clang-tools-extra/trunk/clang-tidy/hicpp/
> MultiwayPathsCoveredCheck.h
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h?rev=318600&view=auto
> ============================================================
> ==================
> --- clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
> (added)
> +++ clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
> Sat Nov 18 11:48:33 2017
> @@ -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 <iostream>
> +
> +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
>
> Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/docs/ReleaseNotes.rst?rev=318600&r1=318599&r2=318600&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
> +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Sat Nov 18 11:48:33 2017
> @@ -158,6 +158,11 @@ Improvements to clang-tidy
> Ensures that all exception will be instances of ``std::exception`` and
> classes
> that are derived from it.
>
> +- New `hicpp-multiway-paths-covered
> + <http://clang.llvm.org/extra/clang-tidy/checks/hicpp-
> multiway-paths-covered.html>`_ check
> +
> + Checks on ``switch`` and ``if`` - ``else if`` constructs that do not
> cover all possible code paths.
> +
> - New `hicpp-signed-bitwise
> <http://clang.llvm.org/extra/clang-tidy/checks/hicpp-
> signed-bitwise.html>`_ check
>
>
> Added: clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-
> multiway-paths-covered.rst
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/docs/clang-tidy/checks/hicpp-multiway-paths-covered.
> rst?rev=318600&view=auto
> ============================================================
> ==================
> --- clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
> (added)
> +++ clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
> Sat Nov 18 11:48:33 2017
> @@ -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 <http://www.codingstandard.com/rule/6-1-2-explicitly-
> cover-all-paths-through-multi-way-selection-statements/>`_
> +and `rule 6.1.4 <http://www.codingstandard.com/rule/6-1-4-ensure-that-a-
> switch-statement-has-at-least-two-case-labels-distinct-from-
> the-default-label/>`_
> +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 <http://www.codingstandard.com/rule/6-1-4-ensure-that-a-
> switch-statement-has-at-least-two-case-labels-distinct-from-
> the-default-label/>`_
> +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`.
>
> Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/docs/clang-tidy/checks/list.rst?rev=318600&r1=318599&
> r2=318600&view=diff
> ============================================================
> ==================
> --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
> +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Sat Nov 18
> 11:48:33 2017
> @@ -81,6 +81,7 @@ Clang-Tidy Checks
> hicpp-invalid-access-moved (redirects to misc-use-after-move)
> <hicpp-invalid-access-moved>
> hicpp-member-init (redirects to cppcoreguidelines-pro-type-member-init)
> <hicpp-member-init>
> hicpp-move-const-arg (redirects to misc-move-const-arg)
> <hicpp-move-const-arg>
> + hicpp-multiway-paths-covered
> hicpp-named-parameter (redirects to readability-named-parameter)
> <hicpp-named-parameter>
> hicpp-new-delete-operators (redirects to misc-new-delete-overloads)
> <hicpp-new-delete-operators>
> hicpp-no-array-decay (redirects to cppcoreguidelines-pro-bounds-array-to-pointer-decay)
> <hicpp-no-array-decay>
>
> Added: clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-
> paths-covered-else.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/test/clang-tidy/hicpp-multiway-paths-covered-else.
> cpp?rev=318600&view=auto
> ============================================================
> ==================
> --- clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
> (added)
> +++ clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
> Sat Nov 18 11:48:33 2017
> @@ -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 */
> + }
> +}
>
> Added: clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-
> paths-covered.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/
> trunk/test/clang-tidy/hicpp-multiway-paths-covered.cpp?
> rev=318600&view=auto
> ============================================================
> ==================
> --- clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered.cpp
> (added)
> +++ clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered.cpp
> Sat Nov 18 11:48:33 2017
> @@ -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 */
> + }
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171120/0af60e4b/attachment-0001.html>
More information about the cfe-commits
mailing list