diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -7,6 +7,7 @@ AvoidGotoCheck.cpp AvoidNonConstGlobalVariablesCheck.cpp CppCoreGuidelinesTidyModule.cpp + DeclareLoopVariableInTheInitializerCheck.cpp InitVariablesCheck.cpp InterfacesGlobalInitCheck.cpp MacroUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -16,6 +16,7 @@ #include "../readability/MagicNumbersCheck.h" #include "AvoidGotoCheck.h" #include "AvoidNonConstGlobalVariablesCheck.h" +#include "DeclareLoopVariableInTheInitializerCheck.h" #include "InitVariablesCheck.h" #include "InterfacesGlobalInitCheck.h" #include "MacroUsageCheck.h" @@ -52,6 +53,8 @@ "cppcoreguidelines-avoid-magic-numbers"); CheckFactories.registerCheck( "cppcoreguidelines-avoid-non-const-global-variables"); + CheckFactories.registerCheck( + "cppcoreguidelines-declare-loop-variable-in-the-initializer"); CheckFactories.registerCheck( "cppcoreguidelines-explicit-virtual-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h @@ -0,0 +1,39 @@ +//===--- DeclareLoopVariableInTheInitializerCheck.h - clang-tidy-*- C++ -*-===// +// +// 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_CPPCOREGUIDELINES_DECLARELOOPVARIABLEINTHEINITIALIZERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_DECLARELOOPVARIABLEINTHEINITIALIZERCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +/// Finds variables that are modified inside a ``for`` statement, are not used +/// elsewhere and are declared outside the ``for`` statement. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.html +class DeclareLoopVariableInTheInitializerCheck : public ClangTidyCheck { +public: + DeclareLoopVariableInTheInitializerCheck(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; + } +}; + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_DECLARELOOPVARIABLEINTHEINITIALIZERCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp @@ -0,0 +1,104 @@ +//===--- DeclareLoopVariableInTheInitializerCheck.cpp - clang-tidy --------===// +// +// 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 "DeclareLoopVariableInTheInitializerCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +class OutsideForStmtVisitor + : public RecursiveASTVisitor { + friend class RecursiveASTVisitor; + +public: + OutsideForStmtVisitor(const ForStmt *ForStmt, const VarDecl *VarDecl, + const MatchFinder::MatchResult &Result) + : MatchedForStmt(ForStmt), MatchedDecl(VarDecl), MatchedResult(Result), + IsOutsideMatchedForStmt(false) {} + + bool hasDeclRefExpOutside(const CompoundStmt *Compound) { + TraverseStmt(const_cast(Compound)); + return IsOutsideMatchedForStmt; + } + +private: + bool VisitDeclRefExpr(DeclRefExpr *D) { + if (const auto *To = dyn_cast(D->getDecl())) { + if (To == MatchedDecl && + !isInsideMatchedForStmt(MatchedResult, DynTypedNode::create(*D))) { + IsOutsideMatchedForStmt = true; + return false; + } + } + return true; + } + + bool isInsideMatchedForStmt(const MatchFinder::MatchResult &Result, + const DynTypedNode &Node) { + const auto *PossibleForStmt = Node.get(); + + if (PossibleForStmt && PossibleForStmt == MatchedForStmt) { + return true; + } + + return llvm::any_of(Result.Context->getParents(Node), + [&](const DynTypedNode &Parent) { + return isInsideMatchedForStmt(Result, Parent); + }); + } + + const ForStmt *MatchedForStmt; + const VarDecl *MatchedDecl; + const MatchFinder::MatchResult &MatchedResult; + bool IsOutsideMatchedForStmt; +}; + +void DeclareLoopVariableInTheInitializerCheck::registerMatchers( + MatchFinder *Finder) { + Finder->addMatcher( + declRefExpr( + hasAncestor(forStmt().bind("ForStmt")), + anyOf(hasAncestor(unaryOperator().bind("Operator")), + hasAncestor( + binaryOperator(isAssignmentOperator()).bind("Operator"))), + to(varDecl(hasAncestor(compoundStmt().bind("Compound")), + unless(hasAncestor(forStmt(equalsBoundNode("ForStmt"))))) + .bind("VarDecl"))), + this); +} + +void DeclareLoopVariableInTheInitializerCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedForStmt = Result.Nodes.getNodeAs("ForStmt"); + const auto *MatchedExprOperator = Result.Nodes.getNodeAs("Operator"); + const auto *MatchedVarDecl = Result.Nodes.getNodeAs("VarDecl"); + const auto *MatchedCompoundStmt = + Result.Nodes.getNodeAs("Compound"); + + if (OutsideForStmtVisitor(MatchedForStmt, MatchedVarDecl, Result) + .hasDeclRefExpOutside(MatchedCompoundStmt)) { + return; + } + + diag(MatchedVarDecl->getLocation(), + "Variable %0 is only modified in a for statement and not used " + "elsewhere. Consider declaring it inside the for statement.") + << MatchedVarDecl; + diag(MatchedExprOperator->getBeginLoc(), "Variable gets modified here", + DiagnosticIDs::Note); +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -89,6 +89,12 @@ Finds inner loops that have not been unrolled, as well as fully unrolled loops with unknown loops bounds or a large number of iterations. +- New :doc:`cppcoreguidelines-declare-loop-variable-in-the-initializer + ` check. + + Finds variables that are modified inside a ``for`` statement, are not used elsewhere + and are declared outside the ``for`` statement. + - New :doc:`cppcoreguidelines-prefer-member-initializer ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.rst @@ -0,0 +1,49 @@ +.. title:: clang-tidy - cppcoreguidelines-declare-loop-variable-in-the-initializer + +cppcoreguidelines-declare-loop-variable-in-the-initializer +========================================================== + +Finds variables that are modified inside a ``for`` statement, are not used elsewhere +and are declared outside the ``for`` statement. + +.. code-block:: c++ + + const int Limit{10}; + + void func1() { + // Should print a warning + int A{0}; + + for(int I{0}; I < Limit; I++) { + A = 3; + } + } + + void func2() { + // Should print a warning + int I{0}; + + for(I = 1; I < Limit; I++) {} + } + + void func3() { + // Should print a warning + int I{0}; + + for(int Unused{0}; I < Limit; I++) {} + } + + void func4() { + // OK + int A{0}; + + for(int I{0}; I < Limit; I++) { + const int B{A}; + } + } + +This check implements the rule `ES.74 `_ of the C++ Core Guidelines. + +It does also cover parts of: + - `ES.5 `_ + - `ES.6 `_ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -143,6 +143,7 @@ `concurrency-thread-canceltype-asynchronous `_, `cppcoreguidelines-avoid-goto `_, `cppcoreguidelines-avoid-non-const-global-variables `_, + `cppcoreguidelines-declare-loop-variable-in-the-initializer `_, `cppcoreguidelines-init-variables `_, "Yes" `cppcoreguidelines-interfaces-global-init `_, `cppcoreguidelines-macro-usage `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-declare-loop-variable-in-the-initializer.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-declare-loop-variable-in-the-initializer.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-declare-loop-variable-in-the-initializer.cpp @@ -0,0 +1,79 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-declare-loop-variable-in-the-initializer %t + +const int Limit{10}; + +void func1() { + // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'A' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer] + // CHECK-MESSAGES: :11:5: note: Variable gets modified here + int A{0}; + + for (int I{0}; I < Limit; I++) { + A = 3; + } +} + +void func2() { + // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'A' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer] + // CHECK-MESSAGES: :21:5: note: Variable gets modified here + int A{0}; + + for (int I{0}; I < Limit; I++) { + A += 3; + } +} + +void func3() { + // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer] + // CHECK-MESSAGES: :30:8: note: Variable gets modified here + int I{0}; + + for (I = 1; I < Limit; I++) { + } +} + +void func4() { + // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer] + // CHECK-MESSAGES: :39:21: note: Variable gets modified here + int I{0}; + + for (; I < Limit; I++) { + } +} + +void func5() { + // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer] + // CHECK-MESSAGES: :48:34: note: Variable gets modified here + int I{0}; + + for (int Unused{0}; I < Limit; I++) { + } +} + +void func6() { + // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: Variable 'I' is only modified in a for statement and not used elsewhere. Consider declaring it inside the for statement. [cppcoreguidelines-declare-loop-variable-in-the-initializer] + // CHECK-MESSAGES: :58:8: note: Variable gets modified here + int I{0}; + int Z{0}; + + for (I = 3; I < Limit; I++) { + } + Z = 3; +} + +void func7() { + // OK + int A{0}; + + for (int I{0}; I < Limit; I++) { + const int B{A}; + } +} + +void func8() { + // OK + int I{0}; + + for (I = 0; I < Limit; I++) { + } + const int A{I}; +}