llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Davide Cunial (capitan-davide) <details> <summary>Changes</summary> Closes https://github.com/llvm/llvm-project/issues/162140 --- Full diff: https://github.com/llvm/llvm-project/pull/162361.diff 9 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.cpp (+89) - (added) clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.h (+41) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.rst (+42) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+2-1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces-consteval-if.cpp (+55) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces.cpp (+123) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index fe261e729539c..e1e42d22c520e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -33,6 +33,7 @@ #include "ImplicitWideningOfMultiplicationResultCheck.h" #include "InaccurateEraseCheck.h" #include "IncDecInConditionsCheck.h" +#include "InconsistentIfelseBracesCheck.h" #include "IncorrectEnableIfCheck.h" #include "IncorrectEnableSharedFromThisCheck.h" #include "IncorrectRoundingsCheck.h" @@ -150,6 +151,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-implicit-widening-of-multiplication-result"); CheckFactories.registerCheck<InaccurateEraseCheck>( "bugprone-inaccurate-erase"); + CheckFactories.registerCheck<InconsistentIfelseBracesCheck>( + "bugprone-inconsistent-ifelse-braces"); CheckFactories.registerCheck<IncorrectEnableIfCheck>( "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 46bc8efd44bc5..d19fd5017d2e0 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -28,6 +28,7 @@ add_clang_library(clangTidyBugproneModule STATIC ForwardingReferenceOverloadCheck.cpp ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp + InconsistentIfelseBracesCheck.cpp IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.cpp new file mode 100644 index 0000000000000..0c6b3e3e3ff9c --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.cpp @@ -0,0 +1,89 @@ +//===----------------------------------------------------------------------===// +// +// 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 "InconsistentIfelseBracesCheck.h" +#include "../utils/BracesAroundStatement.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchers.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +/// Check that at least one branch of the \p If statement is a \c CompoundStmt. +static bool shouldHaveBraces(const IfStmt *If) { + const Stmt *const Then = If->getThen(); + if (isa<CompoundStmt>(Then)) + return true; + + if (const Stmt *const Else = If->getElse()) { + if (const auto *NestedIf = dyn_cast<const IfStmt>(Else)) + return shouldHaveBraces(NestedIf); + + return isa<CompoundStmt>(Else); + } + + return false; +} + +void InconsistentIfelseBracesCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, + ifStmt(hasElse(anything()), + unless(isConsteval()), // 'if consteval' always has braces + unless(hasParent(ifStmt()))) + .bind("if_stmt")), + this); +} + +void InconsistentIfelseBracesCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedIf = Result.Nodes.getNodeAs<IfStmt>("if_stmt"); + if (!shouldHaveBraces(MatchedIf)) + return; + checkIfStmt(Result, MatchedIf); +} + +void InconsistentIfelseBracesCheck::checkIfStmt( + const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If) { + const Stmt *Then = If->getThen(); + if (const auto *NestedIf = dyn_cast<const IfStmt>(Then)) { + // If the then-branch is a nested IfStmt, first we need to add braces to + // it, then we need to check the inner IfStmt. + checkStmt(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc()); + if (shouldHaveBraces(NestedIf)) + checkIfStmt(Result, NestedIf); + } else if (!isa<CompoundStmt>(Then)) { + checkStmt(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc()); + } + + if (const Stmt *const Else = If->getElse()) { + if (const auto *NestedIf = dyn_cast<const IfStmt>(Else)) + checkIfStmt(Result, NestedIf); + else if (!isa<CompoundStmt>(Else)) + checkStmt(Result, If->getElse(), If->getElseLoc()); + } +} + +void InconsistentIfelseBracesCheck::checkStmt( + const ast_matchers::MatchFinder::MatchResult &Result, const Stmt *S, + SourceLocation StartLoc, SourceLocation EndLocHint) { + const SourceManager &SM = *Result.SourceManager; + const LangOptions &LangOpts = Result.Context->getLangOpts(); + + const utils::BraceInsertionHints Hints = + utils::getBraceInsertionsHints(S, LangOpts, SM, StartLoc, EndLocHint); + if (Hints) { + DiagnosticBuilder Diag = diag(Hints.DiagnosticPos, "<message>"); + if (Hints.offersFixIts()) { + Diag << Hints.openingBraceFixIt() << Hints.closingBraceFixIt(); + } + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.h b/clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.h new file mode 100644 index 0000000000000..c818f46fea281 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.h @@ -0,0 +1,41 @@ +//===----------------------------------------------------------------------===// +// +// 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_BUGPRONE_INCONSISTENTIFELSEBRACESCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCONSISTENTIFELSEBRACESCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Detects `if`/`else` statements where one branch uses braces and the other +/// does not. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.html +class InconsistentIfelseBracesCheck : public ClangTidyCheck { +public: + InconsistentIfelseBracesCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + +private: + void checkIfStmt(const ast_matchers::MatchFinder::MatchResult &Result, + const IfStmt *If); + void checkStmt(const ast_matchers::MatchFinder::MatchResult &Result, + const Stmt *S, SourceLocation StartLoc, + SourceLocation EndLocHint = SourceLocation()); +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCONSISTENTIFELSEBRACESCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 7cdff86beeec6..28bebceef1006 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -157,6 +157,12 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`bugprone-inconsistent-ifelse-braces + <clang-tidy/checks/bugprone/inconsistent-ifelse-braces>` check. + + Detects ``if``/``else`` statements where one branch uses braces and the other + does not. + - New :doc:`bugprone-invalid-enum-default-initialization <clang-tidy/checks/bugprone/invalid-enum-default-initialization>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.rst new file mode 100644 index 0000000000000..44f2d64452393 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.rst @@ -0,0 +1,42 @@ +.. title:: clang-tidy - bugprone-inconsistent-ifelse-braces + +bugprone-inconsistent-ifelse-braces +=================================== + +Detects ``if``/``else`` statements where one branch uses braces and the other +does not. + +Before: + +.. code-block:: c++ + + if (condition) { + statement; + } else + statement; + + if (condition) + statement; + + if (condition) + statement; + else + statement; + +After: + +.. code-block:: c++ + + if (condition) { + statement; + } else { + statement; + } + + if (condition) + statement; + + if (condition) + statement; + else + statement; diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c490d2ece2e0a..4c5b70f6c47e5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -101,6 +101,7 @@ Clang-Tidy Checks :doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes" :doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes" :doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`, + :doc:`bugprone-inconsistent-ifelse-braces <bugprone/inconsistent-ifelse-braces>`, "Yes" :doc:`bugprone-incorrect-enable-if <bugprone/incorrect-enable-if>`, "Yes" :doc:`bugprone-incorrect-enable-shared-from-this <bugprone/incorrect-enable-shared-from-this>`, "Yes" :doc:`bugprone-incorrect-roundings <bugprone/incorrect-roundings>`, @@ -256,7 +257,7 @@ Clang-Tidy Checks :doc:`llvm-prefer-static-over-anonymous-namespace <llvm/prefer-static-over-anonymous-namespace>`, :doc:`llvm-twine-local <llvm/twine-local>`, "Yes" :doc:`llvm-use-new-mlir-op-builder <llvm/use-new-mlir-op-builder>`, "Yes" - :doc:`llvm-use-ranges <llvm/use-ranges>`, "Yes" + :doc:`llvm-use-ranges <llvm/use-ranges>`, :doc:`llvmlibc-callee-namespace <llvmlibc/callee-namespace>`, :doc:`llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace>`, :doc:`llvmlibc-inline-function-decl <llvmlibc/inline-function-decl>`, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces-consteval-if.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces-consteval-if.cpp new file mode 100644 index 0000000000000..3eb51c39e2921 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces-consteval-if.cpp @@ -0,0 +1,55 @@ +// RUN: %check_clang_tidy -std=c++23-or-later %s bugprone-inconsistent-ifelse-braces %t + +bool cond(const char *) { return false; } + +void do_something(const char *) {} + +// Positive tests. +void f() { + if consteval { + if (cond("if1")) + do_something("if-single-line"); + else { + } + // CHECK-MESSAGES: :[[@LINE-4]]:21: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if1")) { + // CHECK-FIXES: } else { + } + + if consteval { + if (cond("if2")) + do_something("if-single-line"); + else { + } + // CHECK-MESSAGES: :[[@LINE-4]]:21: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if2")) { + // CHECK-FIXES: } else { + } else { + if (cond("if2.1")) { + } else + do_something("else-single-line"); + // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else { + // CHECK-FIXES: } + } +} + +// Negative tests. +void g() { + if consteval { + if (cond("if0")) { + do_something("if-single-line"); + } else if (cond("if0")) { + do_something("elseif-single-line"); + } else { + do_something("else-single-line"); + } + } else { + if (cond("if0.1")) + do_something("if-single-line"); + else if (cond("if0.1")) + do_something("elseif-single-line"); + else + do_something("else-single-line"); + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces.cpp new file mode 100644 index 0000000000000..3d4af258137ce --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces.cpp @@ -0,0 +1,123 @@ +// RUN: %check_clang_tidy %s bugprone-inconsistent-ifelse-braces %t + +bool cond(const char *) { return false; } + +void do_something(const char *) {} + +// Positive tests. +void f() { + if (cond("if0") /*comment*/) do_something("if-same-line"); + else { + } + // CHECK-MESSAGES: :[[@LINE-3]]:31: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if0") /*comment*/) { do_something("if-same-line"); + // CHECK-FIXES: } else { + + if (cond("if0.1") /*comment*/) { + } else do_something("else-same-line"); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else { do_something("else-same-line"); + // CHECK-FIXES: } + + if (cond("if1")) + do_something("if-single-line"); + else { + } + // CHECK-MESSAGES: :[[@LINE-4]]:19: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if1")) { + // CHECK-FIXES: } else { + + if (cond("if1.1")) { + } else + do_something("else-single-line"); + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else { + // CHECK-FIXES: } + + if (cond("if2") /*comment*/) + // some comment + do_something("if-multi-line"); + else { + } + // CHECK-MESSAGES: :[[@LINE-5]]:31: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if2") /*comment*/) { + // CHECK-FIXES: } else { + + if (cond("if2.1") /*comment*/) { + } else + // some comment + do_something("else-multi-line"); + // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else { + // CHECK-FIXES: } + + if (cond("if3")) do_something("elseif-same-line"); + else if (cond("if3")) { + } else { + } + // CHECK-MESSAGES: :[[@LINE-4]]:19: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if3")) { do_something("elseif-same-line"); + // CHECK-FIXES: } else if (cond("if3")) { + + if (cond("if3.1")) { + } else if (cond("if3.1")) do_something("elseif-same-line"); + else { + } + // CHECK-MESSAGES: :[[@LINE-3]]:28: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else if (cond("if3.1")) { do_something("elseif-same-line"); + // CHECK-FIXES: } else { + + if (cond("if3.2")) { + } else if (cond("if3.2")) { + } else do_something("else-same-line"); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else { do_something("else-same-line"); + // CHECK-FIXES: } + + if (cond("if4-outer")) + if (cond("if4-inner")) + do_something("if-single-line"); + else { + } + else { + } + // CHECK-MESSAGES: :[[@LINE-7]]:25: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-MESSAGES: :[[@LINE-7]]:27: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if4-outer")) { + // CHECK-FIXES: if (cond("if4-inner")) { + // CHECK-FIXES: } else { + // CHECK-FIXES: } else { + + if (cond("if5")) + do_something("if-single-line"); + else if (cond("if5")) { + } + // CHECK-MESSAGES: :[[@LINE-4]]:19: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: if (cond("if5")) { + // CHECK-FIXES: } else if (cond("if5")) { + + if (cond("if5.1")) { + } else if (cond("if5.1")) + do_something("elseif-single-line"); + // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: <message> [bugprone-inconsistent-ifelse-braces] + // CHECK-FIXES: } else if (cond("if5.1")) { + // CHECK-FIXES: } +} + +// Negative tests. +void g() { + if (cond("if0")) { + do_something("if-single-line"); + } else if (cond("if0")) { + do_something("elseif-single-line"); + } else { + do_something("else-single-line"); + } + + if (cond("if1")) + do_something("if-single-line"); + else if (cond("if1")) + do_something("elseif-single-line"); + else + do_something("else-single-line"); +} `````````` </details> https://github.com/llvm/llvm-project/pull/162361 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
