https://github.com/zwuis updated https://github.com/llvm/llvm-project/pull/198237
>From 0445e94537811e0943838124bf810e2f22c647d4 Mon Sep 17 00:00:00 2001 From: Yanzuo Liu <[email protected]> Date: Mon, 18 May 2026 14:15:05 +0800 Subject: [PATCH 1/2] Move part of bugprone-unhandled-code-paths to a new check readability-trivial-switch --- .../bugprone/UnhandledCodePathsCheck.cpp | 43 ++------------- .../bugprone/UnhandledCodePathsCheck.h | 1 - .../clang-tidy/readability/CMakeLists.txt | 1 + .../readability/ReadabilityTidyModule.cpp | 3 ++ .../readability/TrivialSwitchCheck.cpp | 48 +++++++++++++++++ .../readability/TrivialSwitchCheck.h | 33 ++++++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 ++- .../checks/bugprone/unhandled-code-paths.rst | 33 ------------ .../docs/clang-tidy/checks/list.rst | 1 + .../checks/readability/trivial-switch.rst | 36 +++++++++++++ .../bugprone/unhandled-code-paths.cpp | 52 ++----------------- .../checkers/readability/trivial-switch.cpp | 44 ++++++++++++++++ 12 files changed, 178 insertions(+), 123 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/readability/trivial-switch.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/trivial-switch.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp index 1a7d907bac6ec..c02dcd50f10f3 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.cpp @@ -98,39 +98,10 @@ void UnhandledCodePathsCheck::check(const MatchFinder::MatchResult &Result) { bool SwitchHasDefault = false; std::tie(SwitchCaseCount, SwitchHasDefault) = countCaseLabels(Switch); - // 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 (SwitchHasDefault) { - handleSwitchWithDefault(Switch, SwitchCaseCount); + if (SwitchHasDefault || SwitchCaseCount <= 1) return; - } - // Checks all 'switch' statements that do not define a default label. - // Here the heavy lifting happens. - if (!SwitchHasDefault && SwitchCaseCount > 0) { - handleSwitchWithoutDefault(Switch, SwitchCaseCount, Result); - return; - } - // Warns for degenerated 'switch' statements that neither define a case nor - // a default label. - // FIXME: Evaluate, if emitting a fix-it to simplify that statement is - // reasonable. - if (!SwitchHasDefault && SwitchCaseCount == 0) { - diag(Switch->getBeginLoc(), - "switch statement without labels has no effect"); - return; - } - llvm_unreachable("matched a case, that was not explicitly handled"); -} -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) - diag(Switch->getBeginLoc(), - CaseCount == 1 - ? "degenerated switch with default label only" - : "switch could be better written as an if/else statement"); + handleSwitchWithoutDefault(Switch, SwitchCaseCount, Result); } void UnhandledCodePathsCheck::handleSwitchWithoutDefault( @@ -146,11 +117,7 @@ void UnhandledCodePathsCheck::handleSwitchWithoutDefault( // 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. - // 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 " - "separately."); + assert(CaseCount > 1 && "Switch statement with fewer than two cases found."); const std::size_t MaxPathsPossible = [&]() { if (const auto *GeneralCondition = Result.Nodes.getNodeAs<DeclRefExpr>("non-enum-condition")) { @@ -165,10 +132,8 @@ void UnhandledCodePathsCheck::handleSwitchWithoutDefault( return static_cast<std::size_t>(0); }(); - // FIXME: Transform the 'switch' into an 'if' for CaseCount == 1. if (CaseCount < MaxPathsPossible) diag(Switch->getBeginLoc(), - CaseCount == 1 ? "switch with only one case; use an if statement" - : "potential uncovered code path; add a default label"); + "potential uncovered code path; add a default label"); } } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h index 051f1fd66dd63..bb4dfdffb9840 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledCodePathsCheck.h @@ -29,7 +29,6 @@ class UnhandledCodePathsCheck : public ClangTidyCheck { void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: - void handleSwitchWithDefault(const SwitchStmt *Switch, std::size_t CaseCount); void handleSwitchWithoutDefault( const SwitchStmt *Switch, std::size_t CaseCount, const ast_matchers::MatchFinder::MatchResult &Result); diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index 0dfdbe69c880d..c5ee081c351ed 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -61,6 +61,7 @@ add_clang_library(clangTidyReadabilityModule STATIC StringCompareCheck.cpp SuspiciousCallArgumentCheck.cpp TrailingCommaCheck.cpp + TrivialSwitchCheck.cpp UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp UseAnyOfAllOfCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index 86e4f4dcc1de1..3aca2ce7e066d 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -63,6 +63,7 @@ #include "StringCompareCheck.h" #include "SuspiciousCallArgumentCheck.h" #include "TrailingCommaCheck.h" +#include "TrivialSwitchCheck.h" #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" #include "UseAnyOfAllOfCheck.h" @@ -186,6 +187,8 @@ class ReadabilityModule : public ClangTidyModule { "readability-suspicious-call-argument"); CheckFactories.registerCheck<TrailingCommaCheck>( "readability-trailing-comma"); + CheckFactories.registerCheck<TrivialSwitchCheck>( + "readability-trivial-switch"); CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>( "readability-uniqueptr-delete-release"); CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>( diff --git a/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp b/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp new file mode 100644 index 0000000000000..e26a30de4ead1 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp @@ -0,0 +1,48 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "TrivialSwitchCheck.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +void TrivialSwitchCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(switchStmt().bind("switch"), this); +} + +void TrivialSwitchCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Switch = Result.Nodes.getNodeAs<SwitchStmt>("switch"); + std::size_t CaseCount = 0; + bool HasDefault = false; + + for (const SwitchCase *CurrentCase = Switch->getSwitchCaseList(); CurrentCase; + CurrentCase = CurrentCase->getNextSwitchCase()) { + ++CaseCount; + if (isa<DefaultStmt>(CurrentCase)) + HasDefault = true; + } + + // FIXME: Try to add fixit for each case. + switch (SourceLocation Loc = Switch->getBeginLoc(); CaseCount) { + case 0: + diag(Loc, "switch statement without labels has no effect"); + return; + case 1: + if (HasDefault) + diag(Loc, "switch with default label only"); + else + diag(Loc, "switch with only one case; use an if statement"); + return; + case 2: + if (HasDefault) + diag(Loc, "switch could be better written as an if-else statement"); + } +} + +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.h b/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.h new file mode 100644 index 0000000000000..9eff86475932d --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.h @@ -0,0 +1,33 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TRIVIALSWITCHCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TRIVIALSWITCHCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::readability { + +/// Finds trivial switch statements that can be written more clearly. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/readability/trivial-switch.html +class TrivialSwitchCheck : public ClangTidyCheck { +public: + TrivialSwitchCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TRIVIALSWITCHCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 89fb1684bba7c..5d561b62a2412 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -83,8 +83,10 @@ Potentially Breaking Changes <clang-tidy/checks/cppcoreguidelines/pro-type-member-init>` ``hicpp-move-const-arg`` :doc:`performance-move-const-arg <clang-tidy/checks/performance/move-const-arg>` - ``hicpp-multiway-paths-covered`` :doc:`bugprone-unhandled-code-paths - <clang-tidy/checks/bugprone/unhandled-code-paths>` + ``hicpp-multiway-paths-covered`` | :doc:`bugprone-unhandled-code-paths + <clang-tidy/checks/bugprone/unhandled-code-paths>` + | :doc:`readability-trivial-switch + <clang-tidy/checks/readability/trivial-switch>` ``hicpp-named-parameter`` :doc:`readability-named-parameter <clang-tidy/checks/readability/named-parameter>` ``hicpp-new-delete-operators`` :doc:`misc-new-delete-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 index 0e3082afe38f7..01208244989c4 100644 --- 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 @@ -4,8 +4,6 @@ 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. @@ -55,37 +53,6 @@ possible code paths. // 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 ------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 937b80da9b601..0029b83aace91 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -432,6 +432,7 @@ Clang-Tidy Checks :doc:`readability-string-compare <readability/string-compare>`, "Yes" :doc:`readability-suspicious-call-argument <readability/suspicious-call-argument>`, :doc:`readability-trailing-comma <readability/trailing-comma>`, "Yes" + :doc:`readability-trivial-switch <readability/trivial-switch>`, :doc:`readability-uniqueptr-delete-release <readability/uniqueptr-delete-release>`, "Yes" :doc:`readability-uppercase-literal-suffix <readability/uppercase-literal-suffix>`, "Yes" :doc:`readability-use-anyofallof <readability/use-anyofallof>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/trivial-switch.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/trivial-switch.rst new file mode 100644 index 0000000000000..efcee2f0c7caf --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/trivial-switch.rst @@ -0,0 +1,36 @@ +.. title:: clang-tidy - readability-trivial-switch + +readability-trivial-switch +========================== + +Finds trivial ``switch`` statements that can be written more clearly. + +Every ``switch`` statement should have at least two ``case`` labels other than a ``default`` label. +Otherwise, the ``switch`` can be better expressed with an ``if`` statement. +``switch`` statements without any labels are diagnosed as well. + +.. code-block:: c++ + + int i = 42; + + switch (i) { + case 1: + doSomething(); + break; + default: + doSomethingElse(); + break; + } + + // The switch can be written more clearly as: + if (i == 1) { + doSomething(); + } else { + doSomethingElse(); + } + +.. code-block:: c++ + + // The switch without any labels will be diagnosed. + int i = 42; + switch (i) {} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp index 0c3b459ef06fd..b5206db852964 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-code-paths.cpp @@ -12,11 +12,6 @@ struct Bitfields { 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 @@ -28,11 +23,6 @@ void bad_switch(int i) { break; } - // degenerate, maybe even warning - switch (i) { - // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch statement without labels has no effect - } - switch (int j = return_integer()) { // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label case 0: @@ -41,22 +31,6 @@ void bad_switch(int i) { 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 @@ -90,7 +64,10 @@ void bad_switch(int i) { case 1: break; } - // All paths explicitly covered. +} + +void unproblematic_switch(unsigned char c) { + Bitfields Bf; switch (Bf.UInt) { case 0: case 1: @@ -102,13 +79,6 @@ void bad_switch(int i) { 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: @@ -116,18 +86,6 @@ void bad_switch(int i) { } 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) { @@ -136,9 +94,7 @@ void bad_switch(int i) { case false: break; } -} -void unproblematic_switch(unsigned char c) { // switch (c) { case 0: diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trivial-switch.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trivial-switch.cpp new file mode 100644 index 0000000000000..bc693df6599c4 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trivial-switch.cpp @@ -0,0 +1,44 @@ +// RUN: %check_clang_tidy -std=c++98-or-later %s readability-trivial-switch %t + +void bad(int I) { + switch (I) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch statement without labels has no effect [readability-trivial-switch] + } + + switch (I) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with default label only [readability-trivial-switch] + default: + break; + } + + switch (I) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement [readability-trivial-switch] + case 0: + break; + } + + switch (I) { + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as an if-else statement [readability-trivial-switch] + case 0: + break; + default: + break; + } +} + +void good(int I) { + switch (I) { + case 0: + case 1: + break; + } + + switch (I) { + case 0: + break; + case 1: + break; + default: + break; + } +} >From ce59f0bf5d7d12bc96dc6f06ebf8a863a0d237bd Mon Sep 17 00:00:00 2001 From: Yanzuo Liu <[email protected]> Date: Mon, 18 May 2026 14:44:20 +0800 Subject: [PATCH 2/2] Make CI happy --- .../clang-tidy/readability/TrivialSwitchCheck.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp b/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp index e26a30de4ead1..5b5fe4c48724b 100644 --- a/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/TrivialSwitchCheck.cpp @@ -28,8 +28,8 @@ void TrivialSwitchCheck::check(const MatchFinder::MatchResult &Result) { HasDefault = true; } - // FIXME: Try to add fixit for each case. - switch (SourceLocation Loc = Switch->getBeginLoc(); CaseCount) { + // FIXME: Try to add fix-it for each case. + switch (const SourceLocation Loc = Switch->getBeginLoc(); CaseCount) { case 0: diag(Loc, "switch statement without labels has no effect"); return; @@ -42,6 +42,9 @@ void TrivialSwitchCheck::check(const MatchFinder::MatchResult &Result) { case 2: if (HasDefault) diag(Loc, "switch could be better written as an if-else statement"); + [[fallthrough]]; + default: + break; } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
