llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: raindelight <details> <summary>Changes</summary> Part of the work in https://github.com/llvm/llvm-project/issues/183462. Closes https://github.com/llvm/llvm-project/issues/183464. Splitting the check into two more focused checks was considered during discussion, but since clang-tidy does not support one-to-many aliases, a single name covering both behaviors was chosen instead that is more clear than `multiway-paths-covered`. --- Full diff: https://github.com/llvm/llvm-project/pull/191625.diff 12 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (renamed) clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp (+9-10) - (renamed) clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h (+8-8) - (modified) clang-tools-extra/clang-tidy/hicpp/CMakeLists.txt (-1) - (modified) clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp (+2-2) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/unhandled-code-paths.rst (+94) - (modified) clang-tools-extra/docs/clang-tidy/checks/hicpp/multiway-paths-covered.rst (+5-93) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+2) - (renamed) clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths-else.cpp (+2-2) - (renamed) clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp (+1-1) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 07b4c7f3ed518..39859c153b57e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -103,6 +103,7 @@ #include "UncheckedStringToNumberConversionCheck.h" #include "UndefinedMemoryManipulationCheck.h" #include "UndelegatedConstructorCheck.h" +#include "UnhandledCodePathsCheck.h" #include "UnhandledExceptionAtNewCheck.h" #include "UnhandledSelfAssignmentCheck.h" #include "UnintendedCharOstreamOutputCheck.h" @@ -305,6 +306,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-undefined-memory-manipulation"); CheckFactories.registerCheck<UndelegatedConstructorCheck>( "bugprone-undelegated-constructor"); + CheckFactories.registerCheck<UnhandledCodePathsCheck>( + "bugprone-unhandled-code-paths"); CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>( "bugprone-unhandled-self-assignment"); CheckFactories.registerCheck<UnhandledExceptionAtNewCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index bb4e120286727..de27331e54764 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -106,6 +106,7 @@ add_clang_library(clangTidyBugproneModule STATIC UncheckedStringToNumberConversionCheck.cpp UndefinedMemoryManipulationCheck.cpp UndelegatedConstructorCheck.cpp + UnhandledCodePathsCheck.cpp UnhandledExceptionAtNewCheck.cpp UnhandledSelfAssignmentCheck.cpp UniquePtrArrayMismatchCheck.cpp diff --git a/clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp similarity index 92% rename from clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp rename to clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp index 18d5aa44a6a95..1a7d907bac6ec 100644 --- a/clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp @@ -6,21 +6,20 @@ // //===----------------------------------------------------------------------===// -#include "MultiwayPathsCoveredCheck.h" +#include "UnhandledCodePathsCheck.h" #include "clang/AST/ASTContext.h" #include <limits> using namespace clang::ast_matchers; -namespace clang::tidy::hicpp { +namespace clang::tidy::bugprone { -void MultiwayPathsCoveredCheck::storeOptions( - ClangTidyOptions::OptionMap &Opts) { +void UnhandledCodePathsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnMissingElse", WarnOnMissingElse); } -void MultiwayPathsCoveredCheck::registerMatchers(MatchFinder *Finder) { +void UnhandledCodePathsCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( switchStmt( hasCondition(expr( @@ -87,7 +86,7 @@ static std::size_t getNumberOfPossibleValues(QualType T, return 1; } -void MultiwayPathsCoveredCheck::check(const MatchFinder::MatchResult &Result) { +void UnhandledCodePathsCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *ElseIfWithoutElse = Result.Nodes.getNodeAs<IfStmt>("else-if")) { diag(ElseIfWithoutElse->getBeginLoc(), @@ -123,8 +122,8 @@ void MultiwayPathsCoveredCheck::check(const MatchFinder::MatchResult &Result) { llvm_unreachable("matched a case, that was not explicitly handled"); } -void MultiwayPathsCoveredCheck::handleSwitchWithDefault( - const SwitchStmt *Switch, std::size_t CaseCount) { +void UnhandledCodePathsCheck::handleSwitchWithDefault(const SwitchStmt *Switch, + std::size_t CaseCount) { assert(CaseCount > 0 && "Switch statement with supposedly one default " "branch did not contain any case labels"); if (CaseCount == 1 || CaseCount == 2) @@ -134,7 +133,7 @@ void MultiwayPathsCoveredCheck::handleSwitchWithDefault( : "switch could be better written as an if/else statement"); } -void MultiwayPathsCoveredCheck::handleSwitchWithoutDefault( +void UnhandledCodePathsCheck::handleSwitchWithoutDefault( const SwitchStmt *Switch, std::size_t CaseCount, const MatchFinder::MatchResult &Result) { // The matcher only works because some nodes are explicitly matched and @@ -172,4 +171,4 @@ void MultiwayPathsCoveredCheck::handleSwitchWithoutDefault( CaseCount == 1 ? "switch with only one case; use an if statement" : "potential uncovered code path; add a default label"); } -} // namespace clang::tidy::hicpp +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h similarity index 73% rename from clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h rename to clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h index e22e31ac7b05a..051f1fd66dd63 100644 --- a/clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h @@ -6,22 +6,22 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAYPATHSCOVEREDCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAYPATHSCOVEREDCHECK_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDCODEPATHSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDCODEPATHSCHECK_H #include "../ClangTidyCheck.h" -namespace clang::tidy::hicpp { +namespace clang::tidy::bugprone { /// 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: -/// https://clang.llvm.org/extra/clang-tidy/checks/hicpp/multiway-paths-covered.html -class MultiwayPathsCoveredCheck : public ClangTidyCheck { +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unhandled-code-paths.html +class UnhandledCodePathsCheck : public ClangTidyCheck { public: - MultiwayPathsCoveredCheck(StringRef Name, ClangTidyContext *Context) + UnhandledCodePathsCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), WarnOnMissingElse(Options.get("WarnOnMissingElse", false)) {} void storeOptions(ClangTidyOptions::OptionMap &Opts) override; @@ -39,6 +39,6 @@ class MultiwayPathsCoveredCheck : public ClangTidyCheck { const bool WarnOnMissingElse; }; -} // namespace clang::tidy::hicpp +} // namespace clang::tidy::bugprone -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAYPATHSCOVEREDCHECK_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDCODEPATHSCHECK_H diff --git a/clang-tools-extra/clang-tidy/hicpp/CMakeLists.txt b/clang-tools-extra/clang-tidy/hicpp/CMakeLists.txt index b5bcbf389b4ae..18210903e9799 100644 --- a/clang-tools-extra/clang-tidy/hicpp/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/hicpp/CMakeLists.txt @@ -5,7 +5,6 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangTidyHICPPModule STATIC HICPPTidyModule.cpp - MultiwayPathsCoveredCheck.cpp SignedBitwiseCheck.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp b/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp index 38d9235878e62..89ec85fea0b64 100644 --- a/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidyModule.h" #include "../bugprone/StdExceptionBaseclassCheck.h" #include "../bugprone/UndelegatedConstructorCheck.h" +#include "../bugprone/UnhandledCodePathsCheck.h" #include "../bugprone/UnusedReturnValueCheck.h" #include "../bugprone/UseAfterMoveCheck.h" #include "../cppcoreguidelines/AvoidGotoCheck.h" @@ -37,7 +38,6 @@ #include "../readability/FunctionSizeCheck.h" #include "../readability/NamedParameterCheck.h" #include "../readability/UppercaseLiteralSuffixCheck.h" -#include "MultiwayPathsCoveredCheck.h" #include "SignedBitwiseCheck.h" namespace clang::tidy { @@ -59,7 +59,7 @@ class HICPPModule : public ClangTidyModule { "hicpp-exception-baseclass"); CheckFactories.registerCheck<bugprone::UnusedReturnValueCheck>( "hicpp-ignored-remove-result"); - CheckFactories.registerCheck<MultiwayPathsCoveredCheck>( + CheckFactories.registerCheck<bugprone::UnhandledCodePathsCheck>( "hicpp-multiway-paths-covered"); CheckFactories.registerCheck<SignedBitwiseCheck>("hicpp-signed-bitwise"); CheckFactories.registerCheck<google::ExplicitConstructorCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 17a5b006d4b8f..4442cf8b723e0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -211,6 +211,11 @@ New check aliases <clang-tidy/checks/portability/no-assembler>`. The `hicpp-no-assembler` name is kept as an alias. +- Renamed :doc:`hicpp-multiway-paths-covered <clang-tidy/checks/hicpp/multiway-paths-covered>` + to :doc:`bugprone-unhandled-code-paths + <clang-tidy/checks/bugprone/unhandled-code-paths>`. The `hicpp-multiway-paths-covered` + name is kept as an alias + - Renamed :doc:`performance-faster-string-find <clang-tidy/checks/performance/faster-string-find>` to :doc:`performance-prefer-single-char-overloads diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unhandled-code-paths.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unhandled-code-paths.rst new file mode 100644 index 0000000000000..33579f0770308 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unhandled-code-paths.rst @@ -0,0 +1,94 @@ +.. title:: clang-tidy - bugprone-unhandled-code-paths + +bugprone-unhandled-code-paths +============================= + +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. + +``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 + } + + +Every ``switch`` statement should 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 something else here + } + + // Should rather be the following: + if (i == 1) { + // do something here + } + else { + // do something here + } + + +.. code-block:: c++ + + // A completely degenerated switch will be diagnosed. + int i = 42; + switch(i) {} + + +Options +------- + +.. option:: WarnOnMissingElse + + Boolean flag that activates a warning for missing ``else`` branches. + Default is `false`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/hicpp/multiway-paths-covered.rst b/clang-tools-extra/docs/clang-tidy/checks/hicpp/multiway-paths-covered.rst index 13f174778b6de..4650f9ff322a3 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/hicpp/multiway-paths-covered.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/hicpp/multiway-paths-covered.rst @@ -1,98 +1,10 @@ .. title:: clang-tidy - hicpp-multiway-paths-covered +.. meta:: + :http-equiv=refresh: 5;URL=../bugprone/unhandled-code-paths.html 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 <https://www.perforce.com/resources/qac/high-integrity-cpp-coding-rules>`_ -and `rule 6.1.4 <https://www.perforce.com/resources/qac/high-integrity-cpp-coding-rules>`_ -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 <https://www.perforce.com/resources/qac/high-integrity-cpp-coding-rules>`_ -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 something else here - } - - // Should rather be the following: - if (i == 1) { - // do something here - } - else { - // do something here - } - - -.. code-block:: c++ - - // A completely degenerated switch will be diagnosed. - int i = 42; - switch(i) {} - - -Options -------- - -.. option:: WarnOnMissingElse - - Boolean flag that activates a warning for missing ``else`` branches. - Default is `false`. +The `hicpp-multiway-paths-covered` check is an alias, please see +`bugprone-unhandled-code-paths <../bugprone/unhandled-code-paths.html>` +for more information. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 7a64101848971..199a5c2e1dfd3 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -173,6 +173,7 @@ Clang-Tidy Checks :doc:`bugprone-unchecked-string-to-number-conversion <bugprone/unchecked-string-to-number-conversion>`, :doc:`bugprone-undefined-memory-manipulation <bugprone/undefined-memory-manipulation>`, :doc:`bugprone-undelegated-constructor <bugprone/undelegated-constructor>`, + :doc:`bugprone-unhandled-code-paths <bugprone/unhandled-code-paths>`, :doc:`bugprone-unhandled-exception-at-new <bugprone/unhandled-exception-at-new>`, :doc:`bugprone-unhandled-self-assignment <bugprone/unhandled-self-assignment>`, :doc:`bugprone-unintended-char-ostream-output <bugprone/unintended-char-ostream-output>`, "Yes" @@ -611,6 +612,7 @@ Check aliases :doc:`hicpp-ignored-remove-result <hicpp/ignored-remove-result>`, :doc:`bugprone-unused-return-value <bugprone/unused-return-value>`, :doc:`hicpp-invalid-access-moved <hicpp/invalid-access-moved>`, :doc:`bugprone-use-after-move <bugprone/use-after-move>`, :doc:`hicpp-member-init <hicpp/member-init>`, :doc:`cppcoreguidelines-pro-type-member-init <cppcoreguidelines/pro-type-member-init>`, "Yes" + :doc:`hicpp-multiway-paths-covered <hicpp/multiway-paths-covered>`, :doc:`bugprone-unhandled-code-paths <bugprone/unhandled-code-paths>`, :doc:`hicpp-move-const-arg <hicpp/move-const-arg>`, :doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes" :doc:`hicpp-named-parameter <hicpp/named-parameter>`, :doc:`readability-named-parameter <readability/named-parameter>`, "Yes" :doc:`hicpp-new-delete-operators <hicpp/new-delete-operators>`, :doc:`misc-new-delete-overloads <misc/new-delete-overloads>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/hicpp/multiway-paths-covered-else.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths-else.cpp similarity index 92% rename from clang-tools-extra/test/clang-tidy/checkers/hicpp/multiway-paths-covered-else.cpp rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths-else.cpp index 5041b4e97d0c6..420bae681ca65 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/hicpp/multiway-paths-covered-else.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths-else.cpp @@ -1,6 +1,6 @@ -// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t \ +// RUN: %check_clang_tidy %s bugprone-unhandled-code-paths %t \ // RUN: -config='{CheckOptions: \ -// RUN: {hicpp-multiway-paths-covered.WarnOnMissingElse: true}}'\ +// RUN: {bugprone-unhandled-code-paths.WarnOnMissingElse: true}}'\ // RUN: -- enum OS { Mac, diff --git a/clang-tools-extra/test/clang-tidy/checkers/hicpp/multiway-paths-covered.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp similarity index 99% rename from clang-tools-extra/test/clang-tidy/checkers/hicpp/multiway-paths-covered.cpp rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp index 15a3407dc1c27..0c3b459ef06fd 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/hicpp/multiway-paths-covered.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t +// RUN: %check_clang_tidy %s bugprone-unhandled-code-paths %t enum OS { Mac, Windows, `````````` </details> https://github.com/llvm/llvm-project/pull/191625 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
