<div dir="ltr">Hi,<div>We are seeing a bot failure with this commit. Please see the bot page here:</div><div><a href="http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/5470/consoleFull#17462642768254eaf0-7326-4999-85b0-388101f2d404">http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/5470/consoleFull#17462642768254eaf0-7326-4999-85b0-388101f2d404</a><br></div><div><br></div><div>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.</div><div><br></div><div>Respectfully,</div><div>Mike Edwards</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Nov 18, 2017 at 11:48 AM, Jonas Toth via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: jonastoth<br>
Date: Sat Nov 18 11:48:33 2017<br>
New Revision: 318600<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=318600&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=318600&view=rev</a><br>
Log:<br>
[clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches<br>
<br>
Summary:<br>
This check searches for missing `else` branches in `if-else if`-chains and<br>
missing `default` labels in `switch` statements, that use integers as condition.<br>
<br>
It is very similar to -Wswitch, but concentrates on integers only, since enums are<br>
already covered.<br>
<br>
The option to warn for missing `else` branches is deactivated by default, since it is<br>
very noise on larger code bases.<br>
<br>
Running it on LLVM:<br>
{F5354858} for default configuration<br>
{F5354866} just for llvm/lib/Analysis/<wbr>ScalarEvolution.cpp, the else-path checker is very noisy!<br>
<br>
Reviewers: alexfh, aaron.ballman, hokein<br>
<br>
Reviewed By: aaron.ballman<br>
<br>
Subscribers: lebedev.ri, Eugene.Zelenko, cfe-commits, mgorny, JDevlieghere, xazax.hun<br>
<br>
Tags: #clang-tools-extra<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D37808" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37808</a><br>
<br>
<br>
Added:<br>
clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.cpp<br>
clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.h<br>
clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/hicpp-<wbr>multiway-paths-covered.rst<br>
clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered-else.cpp<br>
clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered.cpp<br>
Modified:<br>
clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/CMakeLists.txt<br>
clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/HICPPTidyModule.cpp<br>
clang-tools-extra/trunk/docs/<wbr>ReleaseNotes.rst<br>
clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/list.rst<br>
<br>
Modified: clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt?rev=318600&r1=318599&r2=318600&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clang-tidy/hicpp/<wbr>CMakeLists.txt?rev=318600&r1=<wbr>318599&r2=318600&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/CMakeLists.txt (original)<br>
+++ clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/CMakeLists.txt Sat Nov 18 11:48:33 2017<br>
@@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)<br>
<br>
add_clang_library(<wbr>clangTidyHICPPModule<br>
ExceptionBaseclassCheck.cpp<br>
+ MultiwayPathsCoveredCheck.cpp<br>
NoAssemblerCheck.cpp<br>
HICPPTidyModule.cpp<br>
SignedBitwiseCheck.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/HICPPTidyModule.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp?rev=318600&r1=318599&r2=318600&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clang-tidy/hicpp/<wbr>HICPPTidyModule.cpp?rev=<wbr>318600&r1=318599&r2=318600&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/HICPPTidyModule.cpp (original)<br>
+++ clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/HICPPTidyModule.cpp Sat Nov 18 11:48:33 2017<br>
@@ -35,6 +35,7 @@<br>
#include "../readability/<wbr>FunctionSizeCheck.h"<br>
#include "../readability/<wbr>IdentifierNamingCheck.h"<br>
#include "ExceptionBaseclassCheck.h"<br>
+#include "MultiwayPathsCoveredCheck.h"<br>
#include "NoAssemblerCheck.h"<br>
#include "SignedBitwiseCheck.h"<br>
<br>
@@ -53,6 +54,8 @@ public:<br>
"hicpp-exception-baseclass");<br>
CheckFactories.registerCheck<<wbr>SignedBitwiseCheck>(<br>
"hicpp-signed-bitwise");<br>
+ CheckFactories.registerCheck<<wbr>MultiwayPathsCoveredCheck>(<br>
+ "hicpp-multiway-paths-covered"<wbr>);<br>
CheckFactories.registerCheck<<wbr>google::<wbr>ExplicitConstructorCheck>(<br>
"hicpp-explicit-conversions");<br>
CheckFactories.registerCheck<<wbr>readability::<wbr>FunctionSizeCheck>(<br>
<br>
Added: clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp?rev=318600&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clang-tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.cpp?<wbr>rev=318600&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.cpp (added)<br>
+++ clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.cpp Sat Nov 18 11:48:33 2017<br>
@@ -0,0 +1,179 @@<br>
+//===--- MultiwayPathsCoveredCheck.cpp - clang-tidy--------------------<wbr>----===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
+<br>
+#include "MultiwayPathsCoveredCheck.h"<br>
+#include "clang/AST/ASTContext.h"<br>
+<br>
+#include <limits><br>
+<br>
+using namespace clang::ast_matchers;<br>
+<br>
+namespace clang {<br>
+namespace tidy {<br>
+namespace hicpp {<br>
+<br>
+void MultiwayPathsCoveredCheck::<wbr>storeOptions(<br>
+ ClangTidyOptions::OptionMap &Opts) {<br>
+ Options.store(Opts, "WarnOnMissingElse", WarnOnMissingElse);<br>
+}<br>
+<br>
+void MultiwayPathsCoveredCheck::<wbr>registerMatchers(MatchFinder *Finder) {<br>
+ Finder->addMatcher(<br>
+ stmt(allOf(<br>
+ anyOf(switchStmt(<wbr>forEachSwitchCase(defaultStmt(<wbr>)))<br>
+ .bind("switch-default"),<br>
+ switchStmt(unless(<wbr>hasDescendant(switchCase())))<br>
+ .bind("degenerate-switch"),<br>
+ // This matcher must be the last one of the three<br>
+ // 'switchStmt' options.<br>
+ // Otherwise the cases 'switch-default' and<br>
+ // 'degenerate-switch' are not found correctly.<br>
+ switchStmt(forEachSwitchCase(<wbr>unless(defaultStmt())))<br>
+ .bind("switch-no-default")),<br>
+ switchStmt(hasCondition(allOf(<br>
+ // Match on switch statements that have either a bit-field or an<br>
+ // integer condition. The ordering in 'anyOf()' is important<br>
+ // because the last condition is the most general.<br>
+ anyOf(ignoringImpCasts(<wbr>memberExpr(hasDeclaration(<br>
+ fieldDecl(isBitField()).bind("<wbr>bitfield")))),<br>
+ hasDescendant(declRefExpr().<wbr>bind("non-enum-condition"))),<br>
+ // 'unless()' must be the last match here and must be bound,<br>
+ // otherwise the matcher does not work correctly.<br>
+ unless(hasDescendant(<wbr>declRefExpr(hasType(enumType()<wbr>))<br>
+ .bind("enum-condition"))))))))<wbr>,<br>
+ this);<br>
+<br>
+ // This option is noisy, therefore matching is configurable.<br>
+ if (WarnOnMissingElse) {<br>
+ Finder->addMatcher(<br>
+ ifStmt(allOf(hasParent(ifStmt(<wbr>)), unless(hasElse(anything()))))<br>
+ .bind("else-if"),<br>
+ this);<br>
+ }<br>
+}<br>
+<br>
+static unsigned countCaseLabels(const SwitchStmt *Switch) {<br>
+ unsigned CaseCount = 0;<br>
+<br>
+ const SwitchCase *CurrentCase = Switch->getSwitchCaseList();<br>
+ while (CurrentCase) {<br>
+ ++CaseCount;<br>
+ CurrentCase = CurrentCase-><wbr>getNextSwitchCase();<br>
+ }<br>
+<br>
+ return CaseCount;<br>
+}<br>
+/// This function calculate 2 ** Bits and returns<br>
+/// numeric_limits<std::size_t>::<wbr>max() if an overflow occured.<br>
+static std::size_t twoPow(std::size_t Bits) {<br>
+ return Bits >= std::numeric_limits<std::size_<wbr>t>::digits<br>
+ ? std::numeric_limits<std::size_<wbr>t>::max()<br>
+ : static_cast<size_t>(1) << Bits;<br>
+}<br>
+/// Get the number of possible values that can be switched on for the type T.<br>
+///<br>
+/// \return - 0 if bitcount could not be determined<br>
+/// - numeric_limits<std::size_t>::<wbr>max() when overflow appeared due to<br>
+/// more then 64 bits type size.<br>
+static std::size_t getNumberOfPossibleValues(<wbr>QualType T,<br>
+ const ASTContext &Context) {<br>
+ // `isBooleanType` must come first because `bool` is an integral type as well<br>
+ // and would not return 2 as result.<br>
+ if (T->isBooleanType())<br>
+ return 2;<br>
+ else if (T->isIntegralType(Context))<br>
+ return twoPow(Context.getTypeSize(T))<wbr>;<br>
+ else<br>
+ return 1;<br>
+}<br>
+<br>
+void MultiwayPathsCoveredCheck::<wbr>check(const MatchFinder::MatchResult &Result) {<br>
+ if (const auto *ElseIfWithoutElse =<br>
+ Result.Nodes.getNodeAs<IfStmt><wbr>("else-if")) {<br>
+ diag(ElseIfWithoutElse-><wbr>getLocStart(),<br>
+ "potentially uncovered codepath; add an ending else statement");<br>
+ return;<br>
+ }<br>
+ // Checks the sanity of 'switch' statements that actually do define<br>
+ // a default branch but might be degenerated by having no or only one case.<br>
+ if (const auto *SwitchWithDefault =<br>
+ Result.Nodes.getNodeAs<<wbr>SwitchStmt>("switch-default")) {<br>
+ handleSwitchWithDefault(<wbr>SwitchWithDefault);<br>
+ return;<br>
+ }<br>
+ // Checks all 'switch' statements that do not define a default label.<br>
+ // Here the heavy lifting happens.<br>
+ if (const auto *SwitchWithoutDefault =<br>
+ Result.Nodes.getNodeAs<<wbr>SwitchStmt>("switch-no-<wbr>default")) {<br>
+ handleSwitchWithoutDefault(<wbr>SwitchWithoutDefault, Result);<br>
+ return;<br>
+ }<br>
+ // Warns for degenerated 'switch' statements that neither define a case nor<br>
+ // a default label.<br>
+ if (const auto *DegenerateSwitch =<br>
+ Result.Nodes.getNodeAs<<wbr>SwitchStmt>("degenerate-<wbr>switch")) {<br>
+ diag(DegenerateSwitch-><wbr>getLocStart(), "degenerated switch without labels");<br>
+ return;<br>
+ }<br>
+ llvm_unreachable("matched a case, that was not explicitly handled");<br>
+}<br>
+<br>
+void MultiwayPathsCoveredCheck::<wbr>handleSwitchWithDefault(<br>
+ const SwitchStmt *Switch) {<br>
+ unsigned CaseCount = countCaseLabels(Switch);<br>
+ assert(CaseCount > 0 && "Switch statement with supposedly one default "<br>
+ "branch did not contain any case labels");<br>
+ if (CaseCount == 1 || CaseCount == 2)<br>
+ diag(Switch->getLocStart(),<br>
+ CaseCount == 1<br>
+ ? "degenerated switch with default label only"<br>
+ : "switch could be better written as an if/else statement");<br>
+}<br>
+<br>
+void MultiwayPathsCoveredCheck::<wbr>handleSwitchWithoutDefault(<br>
+ const SwitchStmt *Switch, const MatchFinder::MatchResult &Result) {<br>
+ // The matcher only works because some nodes are explicitly matched and<br>
+ // bound but ignored. This is necessary to build the excluding logic for<br>
+ // enums and 'switch' statements without a 'default' branch.<br>
+ assert(!Result.Nodes.<wbr>getNodeAs<DeclRefExpr>("enum-<wbr>condition") &&<br>
+ "switch over enum is handled by warnings already, explicitly ignoring "<br>
+ "them");<br>
+ assert(!Result.Nodes.<wbr>getNodeAs<SwitchStmt>("switch-<wbr>default") &&<br>
+ "switch stmts with default branch do cover all paths, explicitly "<br>
+ "ignoring them");<br>
+ // Determine the number of case labels. Since 'default' is not present<br>
+ // and duplicating case labels is not allowed this number represents<br>
+ // the number of codepaths. It can be directly compared to 'MaxPathsPossible'<br>
+ // to see if some cases are missing.<br>
+ unsigned CaseCount = countCaseLabels(Switch);<br>
+ // CaseCount == 0 is caught in DegenerateSwitch. Necessary because the<br>
+ // matcher used for here does not match on degenerate 'switch'.<br>
+ assert(CaseCount > 0 && "Switch statement without any case found. This case "<br>
+ "should be excluded by the matcher and is handled "<br>
+ "separatly.");<br>
+ std::size_t MaxPathsPossible = [&]() {<br>
+ if (const auto *GeneralCondition =<br>
+ Result.Nodes.getNodeAs<<wbr>DeclRefExpr>("non-enum-<wbr>condition"))<br>
+ return getNumberOfPossibleValues(<wbr>GeneralCondition->getType(),<br>
+ *Result.Context);<br>
+ if (const auto *BitfieldDecl =<br>
+ Result.Nodes.getNodeAs<<wbr>FieldDecl>("bitfield"))<br>
+ return twoPow(BitfieldDecl-><wbr>getBitWidthValue(*Result.<wbr>Context));<br>
+ llvm_unreachable("either bit-field or non-enum must be condition");<br>
+ }();<br>
+<br>
+ // FIXME: Transform the 'switch' into an 'if' for CaseCount == 1.<br>
+ if (CaseCount < MaxPathsPossible)<br>
+ diag(Switch->getLocStart(),<br>
+ CaseCount == 1 ? "switch with only one case; use an if statement"<br>
+ : "potential uncovered code path; add a default label");<br>
+}<br>
+} // namespace hicpp<br>
+} // namespace tidy<br>
+} // namespace clang<br>
<br>
Added: clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h?rev=318600&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/clang-tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.h?<wbr>rev=318600&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.h (added)<br>
+++ clang-tools-extra/trunk/clang-<wbr>tidy/hicpp/<wbr>MultiwayPathsCoveredCheck.h Sat Nov 18 11:48:33 2017<br>
@@ -0,0 +1,51 @@<br>
+//===--- MultiwayPathsCoveredCheck.h - clang-tidy----------------*- C++ -*-===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
+<br>
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_<wbr>TIDY_HICPP_MULTIWAY_PATHS_<wbr>COVERED_H<br>
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_<wbr>TIDY_HICPP_MULTIWAY_PATHS_<wbr>COVERED_H<br>
+<br>
+#include "../ClangTidy.h"<br>
+#include "clang/ASTMatchers/<wbr>ASTMatchFinder.h"<br>
+#include <iostream><br>
+<br>
+namespace clang {<br>
+namespace tidy {<br>
+namespace hicpp {<br>
+<br>
+/// Find occasions where not all codepaths are explicitly covered in code.<br>
+/// This includes 'switch' without a 'default'-branch and 'if'-'else if'-chains<br>
+/// without a final 'else'-branch.<br>
+///<br>
+/// For the user-facing documentation see:<br>
+/// <a href="http://clang.llvm.org/extra/clang-tidy/checks/hicpp-multiway-paths-covered.html" rel="noreferrer" target="_blank">http://clang.llvm.org/extra/<wbr>clang-tidy/checks/hicpp-<wbr>multiway-paths-covered.html</a><br>
+class MultiwayPathsCoveredCheck : public ClangTidyCheck {<br>
+public:<br>
+ MultiwayPathsCoveredCheck(<wbr>StringRef Name, ClangTidyContext *Context)<br>
+ : ClangTidyCheck(Name, Context),<br>
+ WarnOnMissingElse(Options.get(<wbr>"WarnOnMissingElse", 0)) {}<br>
+ void storeOptions(ClangTidyOptions:<wbr>:OptionMap &Opts) override;<br>
+ void registerMatchers(ast_matchers:<wbr>:MatchFinder *Finder) override;<br>
+ void check(const ast_matchers::MatchFinder::<wbr>MatchResult &Result) override;<br>
+<br>
+private:<br>
+ void handleSwitchWithDefault(const SwitchStmt *Switch);<br>
+ void handleSwitchWithoutDefault(<br>
+ const SwitchStmt *Switch,<br>
+ const ast_matchers::MatchFinder::<wbr>MatchResult &Result);<br>
+ /// This option can be configured to warn on missing 'else' branches in an<br>
+ /// 'if-else if' chain. The default is false because this option might be<br>
+ /// noisy on some code bases.<br>
+ const bool WarnOnMissingElse;<br>
+};<br>
+<br>
+} // namespace hicpp<br>
+} // namespace tidy<br>
+} // namespace clang<br>
+<br>
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_<wbr>TIDY_HICPP_MULTIWAY_PATHS_<wbr>COVERED_H<br>
<br>
Modified: clang-tools-extra/trunk/docs/<wbr>ReleaseNotes.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=318600&r1=318599&r2=318600&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/docs/ReleaseNotes.rst?<wbr>rev=318600&r1=318599&r2=<wbr>318600&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/docs/<wbr>ReleaseNotes.rst (original)<br>
+++ clang-tools-extra/trunk/docs/<wbr>ReleaseNotes.rst Sat Nov 18 11:48:33 2017<br>
@@ -158,6 +158,11 @@ Improvements to clang-tidy<br>
Ensures that all exception will be instances of ``std::exception`` and classes<br>
that are derived from it.<br>
<br>
+- New `hicpp-multiway-paths-covered<br>
+ <<a href="http://clang.llvm.org/extra/clang-tidy/checks/hicpp-multiway-paths-covered.html" rel="noreferrer" target="_blank">http://clang.llvm.org/extra/<wbr>clang-tidy/checks/hicpp-<wbr>multiway-paths-covered.html</a>>`_ check<br>
+<br>
+ Checks on ``switch`` and ``if`` - ``else if`` constructs that do not cover all possible code paths.<br>
+<br>
- New `hicpp-signed-bitwise<br>
<<a href="http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html" rel="noreferrer" target="_blank">http://clang.llvm.org/extra/<wbr>clang-tidy/checks/hicpp-<wbr>signed-bitwise.html</a>>`_ check<br>
<br>
<br>
Added: clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/hicpp-<wbr>multiway-paths-covered.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst?rev=318600&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/docs/clang-tidy/checks/<wbr>hicpp-multiway-paths-covered.<wbr>rst?rev=318600&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/hicpp-<wbr>multiway-paths-covered.rst (added)<br>
+++ clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/hicpp-<wbr>multiway-paths-covered.rst Sat Nov 18 11:48:33 2017<br>
@@ -0,0 +1,95 @@<br>
+.. title:: clang-tidy - hicpp-multiway-paths-covered<br>
+<br>
+hicpp-multiway-paths-covered<br>
+============================<br>
+<br>
+This check discovers situations where code paths are not fully-covered.<br>
+It furthermore suggests using ``if`` instead of ``switch`` if the code will be more clear.<br>
+The `rule 6.1.2 <<a href="http://www.codingstandard.com/rule/6-1-2-explicitly-cover-all-paths-through-multi-way-selection-statements/" rel="noreferrer" target="_blank">http://www.codingstandard.<wbr>com/rule/6-1-2-explicitly-<wbr>cover-all-paths-through-multi-<wbr>way-selection-statements/</a>>`_<br>
+and `rule 6.1.4 <<a href="http://www.codingstandard.com/rule/6-1-4-ensure-that-a-switch-statement-has-at-least-two-case-labels-distinct-from-the-default-label/" rel="noreferrer" target="_blank">http://www.codingstandard.<wbr>com/rule/6-1-4-ensure-that-a-<wbr>switch-statement-has-at-least-<wbr>two-case-labels-distinct-from-<wbr>the-default-label/</a>>`_<br>
+of the High Integrity C++ Coding Standard are enforced.<br>
+<br>
+``if-else if`` chains that miss a final ``else`` branch might lead to unexpected<br>
+program execution and be the result of a logical error.<br>
+If the missing ``else`` branch is intended you can leave it empty with a clarifying<br>
+comment.<br>
+This warning can be noisy on some code bases, so it is disabled by default.<br>
+<br>
+.. code-block:: c++<br>
+<br>
+ void f1() {<br>
+ int i = determineTheNumber();<br>
+<br>
+ if(i > 0) {<br>
+ // Some Calculation<br>
+ } else if (i < 0) {<br>
+ // Precondition violated or something else.<br>
+ }<br>
+ // ...<br>
+ }<br>
+<br>
+Similar arguments hold for ``switch`` statements which do not cover all possible code paths.<br>
+<br>
+.. code-block:: c++<br>
+<br>
+ // The missing default branch might be a logical error. It can be kept empty<br>
+ // if there is nothing to do, making it explicit.<br>
+ void f2(int i) {<br>
+ switch (i) {<br>
+ case 0: // something<br>
+ break;<br>
+ case 1: // something else<br>
+ break;<br>
+ }<br>
+ // All other numbers?<br>
+ }<br>
+<br>
+ // Violates this rule as well, but already emits a compiler warning (-Wswitch).<br>
+ enum Color { Red, Green, Blue, Yellow };<br>
+ void f3(enum Color c) {<br>
+ switch (c) {<br>
+ case Red: // We can't drive for now.<br>
+ break;<br>
+ case Green: // We are allowed to drive.<br>
+ break;<br>
+ }<br>
+ // Other cases missing<br>
+ }<br>
+<br>
+<br>
+The `rule 6.1.4 <<a href="http://www.codingstandard.com/rule/6-1-4-ensure-that-a-switch-statement-has-at-least-two-case-labels-distinct-from-the-default-label/" rel="noreferrer" target="_blank">http://www.codingstandard.<wbr>com/rule/6-1-4-ensure-that-a-<wbr>switch-statement-has-at-least-<wbr>two-case-labels-distinct-from-<wbr>the-default-label/</a>>`_<br>
+requires every ``switch`` statement to have at least two ``case`` labels other than a `default` label.<br>
+Otherwise, the ``switch`` could be better expressed with an ``if`` statement.<br>
+Degenerated ``switch`` statements without any labels are caught as well.<br>
+<br>
+.. code-block:: c++<br>
+<br>
+ // Degenerated switch that could be better written as `if`<br>
+ int i = 42;<br>
+ switch(i) {<br>
+ case 1: // do something here<br>
+ default: // do somethe else here<br>
+ }<br>
+<br>
+ // Should rather be the following:<br>
+ if (i == 1) {<br>
+ // do something here<br>
+ }<br>
+ else {<br>
+ // do something here<br>
+ }<br>
+<br>
+<br>
+.. code-block:: c++<br>
+<br>
+ // A completly degenerated switch will be diagnosed.<br>
+ int i = 42;<br>
+ switch(i) {}<br>
+<br>
+<br>
+Options<br>
+-------<br>
+<br>
+.. option:: WarnOnMissingElse<br>
+<br>
+ Activates warning for missing``else`` branches. Default is `0`.<br>
<br>
Modified: clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/list.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=318600&r1=318599&r2=318600&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/docs/clang-tidy/checks/<wbr>list.rst?rev=318600&r1=318599&<wbr>r2=318600&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/list.rst (original)<br>
+++ clang-tools-extra/trunk/docs/<wbr>clang-tidy/checks/list.rst Sat Nov 18 11:48:33 2017<br>
@@ -81,6 +81,7 @@ Clang-Tidy Checks<br>
hicpp-invalid-access-moved (redirects to misc-use-after-move) <hicpp-invalid-access-moved><br>
hicpp-member-init (redirects to cppcoreguidelines-pro-type-<wbr>member-init) <hicpp-member-init><br>
hicpp-move-const-arg (redirects to misc-move-const-arg) <hicpp-move-const-arg><br>
+ hicpp-multiway-paths-covered<br>
hicpp-named-parameter (redirects to readability-named-parameter) <hicpp-named-parameter><br>
hicpp-new-delete-operators (redirects to misc-new-delete-overloads) <hicpp-new-delete-operators><br>
hicpp-no-array-decay (redirects to cppcoreguidelines-pro-bounds-<wbr>array-to-pointer-decay) <hicpp-no-array-decay><br>
<br>
Added: clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered-else.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered-else.cpp?rev=318600&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/test/clang-tidy/hicpp-<wbr>multiway-paths-covered-else.<wbr>cpp?rev=318600&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered-else.cpp (added)<br>
+++ clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered-else.cpp Sat Nov 18 11:48:33 2017<br>
@@ -0,0 +1,57 @@<br>
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t \<br>
+// RUN: -config='{CheckOptions: \<br>
+// RUN: [{key: hicpp-multiway-paths-covered.<wbr>WarnOnMissingElse, value: 1}]}'\<br>
+// RUN: --<br>
+<br>
+enum OS { Mac,<br>
+ Windows,<br>
+ Linux };<br>
+<br>
+void problematic_if(int i, enum OS os) {<br>
+ if (i > 0) {<br>
+ return;<br>
+ } else if (i < 0) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:10: warning: potentially uncovered codepath; add an ending else statement<br>
+ return;<br>
+ }<br>
+<br>
+ // Could be considered as false positive because all paths are covered logically.<br>
+ // I still think this is valid since the possibility of a final 'everything else'<br>
+ // codepath is expected from if-else if.<br>
+ if (i > 0) {<br>
+ return;<br>
+ } else if (i <= 0) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:10: warning: potentially uncovered codepath; add an ending else statement<br>
+ return;<br>
+ }<br>
+<br>
+ // Test if nesting of if-else chains does get caught as well.<br>
+ if (os == Mac) {<br>
+ return;<br>
+ } else if (os == Linux) {<br>
+ // These checks are kind of degenerated, but the check will not try to solve<br>
+ // if logically all paths are covered, which is more the area of the static analyzer.<br>
+ if (true) {<br>
+ return;<br>
+ } else if (false) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:12: warning: potentially uncovered codepath; add an ending else statement<br>
+ return;<br>
+ }<br>
+ return;<br>
+ } else {<br>
+ /* unreachable */<br>
+ if (true) // check if the parent would match here as well<br>
+ return;<br>
+ // No warning for simple if statements, since it is common to just test one condition<br>
+ // and ignore the opposite.<br>
+ }<br>
+<br>
+ // Ok, because all paths are covered<br>
+ if (i > 0) {<br>
+ return;<br>
+ } else if (i < 0) {<br>
+ return;<br>
+ } else {<br>
+ /* error, maybe precondition failed */<br>
+ }<br>
+}<br>
<br>
Added: clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered.cpp?rev=318600&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/clang-tools-extra/<wbr>trunk/test/clang-tidy/hicpp-<wbr>multiway-paths-covered.cpp?<wbr>rev=318600&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered.cpp (added)<br>
+++ clang-tools-extra/trunk/test/<wbr>clang-tidy/hicpp-multiway-<wbr>paths-covered.cpp Sat Nov 18 11:48:33 2017<br>
@@ -0,0 +1,467 @@<br>
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t<br>
+<br>
+enum OS { Mac,<br>
+ Windows,<br>
+ Linux };<br>
+<br>
+struct Bitfields {<br>
+ unsigned UInt : 3;<br>
+ int SInt : 1;<br>
+};<br>
+<br>
+int return_integer() { return 42; }<br>
+<br>
+void bad_switch(int i) {<br>
+ switch (i) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement<br>
+ case 0:<br>
+ break;<br>
+ }<br>
+ // No default in this switch<br>
+ switch (i) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label<br>
+ case 0:<br>
+ break;<br>
+ case 1:<br>
+ break;<br>
+ case 2:<br>
+ break;<br>
+ }<br>
+<br>
+ // degenerate, maybe even warning<br>
+ switch (i) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels<br>
+ }<br>
+<br>
+ switch (int j = return_integer()) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label<br>
+ case 0:<br>
+ case 1:<br>
+ case 2:<br>
+ break;<br>
+ }<br>
+<br>
+ // Degenerated, only default case.<br>
+ switch (i) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only<br>
+ default:<br>
+ break;<br>
+ }<br>
+<br>
+ // Degenerated, only one case label and default case -> Better as if-stmt.<br>
+ switch (i) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as an if/else statement<br>
+ case 0:<br>
+ break;<br>
+ default:<br>
+ break;<br>
+ }<br>
+<br>
+ unsigned long long BigNumber = 0;<br>
+ switch (BigNumber) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label<br>
+ case 0:<br>
+ case 1:<br>
+ break;<br>
+ }<br>
+<br>
+ const int &IntRef = i;<br>
+ switch (IntRef) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label<br>
+ case 0:<br>
+ case 1:<br>
+ break;<br>
+ }<br>
+<br>
+ char C = 'A';<br>
+ switch (C) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label<br>
+ case 'A':<br>
+ break;<br>
+ case 'B':<br>
+ break;<br>
+ }<br>
+<br>
+ Bitfields Bf;<br>
+ // UInt has 3 bits size.<br>
+ switch (Bf.UInt) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label<br>
+ case 0:<br>
+ case 1:<br>
+ break;<br>
+ }<br>
+ // All paths explicitly covered.<br>
+ switch (Bf.UInt) {<br>
+ case 0:<br>
+ case 1:<br>
+ case 2:<br>
+ case 3:<br>
+ case 4:<br>
+ case 5:<br>
+ case 6:<br>
+ case 7:<br>
+ break;<br>
+ }<br>
+ // SInt has 1 bit size, so this is somewhat degenerated.<br>
+ switch (Bf.SInt) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement<br>
+ case 0:<br>
+ break;<br>
+ }<br>
+ // All paths explicitly covered.<br>
+ switch (Bf.SInt) {<br>
+ case 0:<br>
+ case 1:<br>
+ break;<br>
+ }<br>
+<br>
+ bool Flag = false;<br>
+ switch (Flag) {<br>
+ // CHECK-MESSAGES:[[@LINE-1]]:3: warning: switch with only one case; use an if statement<br>
+ case true:<br>
+ break;<br>
+ }<br>
+<br>
+ switch (Flag) {<br>
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only<br>
+ default:<br>
+ break;<br>
+ }<br>
+<br>
+ // This `switch` will create a frontend warning from '-Wswitch-bool' but is<br>
+ // ok for this check.<br>
+ switch (Flag) {<br>
+ case true:<br>
+ break;<br>
+ case false:<br>
+ break;<br>
+ }<br>
+}<br>
+<br>
+void unproblematic_switch(unsigned char c) {<br>
+ switch (c) {<br>
+ case 0:<br>
+ case 1:<br>
+ case 2:<br>
+ case 3:<br>
+ case 4:<br>
+ case 5:<br>
+ case 6:<br>
+ case 7:<br>
+ case 8:<br>
+ case 9:<br>
+ case 10:<br>
+ case 11:<br>
+ case 12:<br>
+ case 13:<br>
+ case 14:<br>
+ case 15:<br>
+ case 16:<br>
+ case 17:<br>
+ case 18:<br>
+ case 19:<br>
+ case 20:<br>
+ case 21:<br>
+ case 22:<br>
+ case 23:<br>
+ case 24:<br>
+ case 25:<br>
+ case 26:<br>
+ case 27:<br>
+ case 28:<br>
+ case 29:<br>
+ case 30:<br>
+ case 31:<br>
+ case 32:<br>
+ case 33:<br>
+ case 34:<br>
+ case 35:<br>
+ case 36:<br>
+ case 37:<br>
+ case 38:<br>
+ case 39:<br>
+ case 40:<br>
+ case 41:<br>
+ case 42:<br>
+ case 43:<br>
+ case 44:<br>
+ case 45:<br>
+ case 46:<br>
+ case 47:<br>
+ case 48:<br>
+ case 49:<br>
+ case 50:<br>
+ case 51:<br>
+ case 52:<br>
+ case 53:<br>
+ case 54:<br>
+ case 55:<br>
+ case 56:<br>
+ case 57:<br>
+ case 58:<br>
+ case 59:<br>
+ case 60:<br>
+ case 61:<br>
+ case 62:<br>
+ case 63:<br>
+ case 64:<br>
+ case 65:<br>
+ case 66:<br>
+ case 67:<br>
+ case 68:<br>
+ case 69:<br>
+ case 70:<br>
+ case 71:<br>
+ case 72:<br>
+ case 73:<br>
+ case 74:<br>
+ case 75:<br>
+ case 76:<br>
+ case 77:<br>
+ case 78:<br>
+ case 79:<br>
+ case 80:<br>
+ case 81:<br>
+ case 82:<br>
+ case 83:<br>
+ case 84:<br>
+ case 85:<br>
+ case 86:<br>
+ case 87:<br>
+ case 88:<br>
+ case 89:<br>
+ case 90:<br>
+ case 91:<br>
+ case 92:<br>
+ case 93:<br>
+ case 94:<br>
+ case 95:<br>
+ case 96:<br>
+ case 97:<br>
+ case 98:<br>
+ case 99:<br>
+ case 100:<br>
+ case 101:<br>
+ case 102:<br>
+ case 103:<br>
+ case 104:<br>
+ case 105:<br>
+ case 106:<br>
+ case 107:<br>
+ case 108:<br>
+ case 109:<br>
+ case 110:<br>
+ case 111:<br>
+ case 112:<br>
+ case 113:<br>
+ case 114:<br>
+ case 115:<br>
+ case 116:<br>
+ case 117:<br>
+ case 118:<br>
+ case 119:<br>
+ case 120:<br>
+ case 121:<br>
+ case 122:<br>
+ case 123:<br>
+ case 124:<br>
+ case 125:<br>
+ case 126:<br>
+ case 127:<br>
+ case 128:<br>
+ case 129:<br>
+ case 130:<br>
+ case 131:<br>
+ case 132:<br>
+ case 133:<br>
+ case 134:<br>
+ case 135:<br>
+ case 136:<br>
+ case 137:<br>
+ case 138:<br>
+ case 139:<br>
+ case 140:<br>
+ case 141:<br>
+ case 142:<br>
+ case 143:<br>
+ case 144:<br>
+ case 145:<br>
+ case 146:<br>
+ case 147:<br>
+ case 148:<br>
+ case 149:<br>
+ case 150:<br>
+ case 151:<br>
+ case 152:<br>
+ case 153:<br>
+ case 154:<br>
+ case 155:<br>
+ case 156:<br>
+ case 157:<br>
+ case 158:<br>
+ case 159:<br>
+ case 160:<br>
+ case 161:<br>
+ case 162:<br>
+ case 163:<br>
+ case 164:<br>
+ case 165:<br>
+ case 166:<br>
+ case 167:<br>
+ case 168:<br>
+ case 169:<br>
+ case 170:<br>
+ case 171:<br>
+ case 172:<br>
+ case 173:<br>
+ case 174:<br>
+ case 175:<br>
+ case 176:<br>
+ case 177:<br>
+ case 178:<br>
+ case 179:<br>
+ case 180:<br>
+ case 181:<br>
+ case 182:<br>
+ case 183:<br>
+ case 184:<br>
+ case 185:<br>
+ case 186:<br>
+ case 187:<br>
+ case 188:<br>
+ case 189:<br>
+ case 190:<br>
+ case 191:<br>
+ case 192:<br>
+ case 193:<br>
+ case 194:<br>
+ case 195:<br>
+ case 196:<br>
+ case 197:<br>
+ case 198:<br>
+ case 199:<br>
+ case 200:<br>
+ case 201:<br>
+ case 202:<br>
+ case 203:<br>
+ case 204:<br>
+ case 205:<br>
+ case 206:<br>
+ case 207:<br>
+ case 208:<br>
+ case 209:<br>
+ case 210:<br>
+ case 211:<br>
+ case 212:<br>
+ case 213:<br>
+ case 214:<br>
+ case 215:<br>
+ case 216:<br>
+ case 217:<br>
+ case 218:<br>
+ case 219:<br>
+ case 220:<br>
+ case 221:<br>
+ case 222:<br>
+ case 223:<br>
+ case 224:<br>
+ case 225:<br>
+ case 226:<br>
+ case 227:<br>
+ case 228:<br>
+ case 229:<br>
+ case 230:<br>
+ case 231:<br>
+ case 232:<br>
+ case 233:<br>
+ case 234:<br>
+ case 235:<br>
+ case 236:<br>
+ case 237:<br>
+ case 238:<br>
+ case 239:<br>
+ case 240:<br>
+ case 241:<br>
+ case 242:<br>
+ case 243:<br>
+ case 244:<br>
+ case 245:<br>
+ case 246:<br>
+ case 247:<br>
+ case 248:<br>
+ case 249:<br>
+ case 250:<br>
+ case 251:<br>
+ case 252:<br>
+ case 253:<br>
+ case 254:<br>
+ case 255:<br>
+ break;<br>
+ }<br>
+<br>
+ // Some paths are covered by the switch and a default case is present.<br>
+ switch (c) {<br>
+ case 1:<br>
+ case 2:<br>
+ case 3:<br>
+ default:<br>
+ break;<br>
+ }<br>
+}<br>
+<br>
+OS return_enumerator() {<br>
+ return Linux;<br>
+}<br>
+<br>
+// Enumpaths are already covered by a warning, this is just to ensure, that there is<br>
+// no interference or false positives.<br>
+// -Wswitch warns about uncovered enum paths and each here described case is already<br>
+// covered.<br>
+void switch_enums(OS os) {<br>
+ switch (os) {<br>
+ case Linux:<br>
+ break;<br>
+ }<br>
+<br>
+ switch (OS another_os = return_enumerator()) {<br>
+ case Linux:<br>
+ break;<br>
+ }<br>
+<br>
+ switch (os) {<br>
+ }<br>
+}<br>
+<br>
+/// All of these cases will not emit a warning per default, but with explicit activation.<br>
+/// Covered in extra test file.<br>
+void problematic_if(int i, enum OS os) {<br>
+ if (i > 0) {<br>
+ return;<br>
+ } else if (i < 0) {<br>
+ return;<br>
+ }<br>
+<br>
+ if (os == Mac) {<br>
+ return;<br>
+ } else if (os == Linux) {<br>
+ if (true) {<br>
+ return;<br>
+ } else if (false) {<br>
+ return;<br>
+ }<br>
+ return;<br>
+ } else {<br>
+ /* unreachable */<br>
+ if (true) // check if the parent would match here as well<br>
+ return;<br>
+ }<br>
+<br>
+ // Ok, because all paths are covered<br>
+ if (i > 0) {<br>
+ return;<br>
+ } else if (i < 0) {<br>
+ return;<br>
+ } else {<br>
+ /* error, maybe precondition failed */<br>
+ }<br>
+}<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>