Index: clang-tidy/performance/CMakeLists.txt =================================================================== --- clang-tidy/performance/CMakeLists.txt +++ clang-tidy/performance/CMakeLists.txt @@ -1,6 +1,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyPerformanceModule + ForRangeCopyCheck.cpp PerformanceTidyModule.cpp UnnecessaryCopyInitialization.cpp Index: clang-tidy/performance/ForRangeCopyCheck.h =================================================================== --- /dev/null +++ clang-tidy/performance/ForRangeCopyCheck.h @@ -0,0 +1,46 @@ +//===--- ForRangeCopyCheck.h - clang-tidy------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FORRANGECOPYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FORRANGECOPYCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace performance { + +// A check that detects copied loop variables and suggests using const +// references. +class ForRangeCopyCheck : public ClangTidyCheck { +public: + ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + // Checks if the loop variable is a const value and expensive to copy. If so + // suggests it be converted to a const reference. + bool handleConstValueCopy(const VarDecl &LoopVar, ASTContext &Context); + // Checks if the loop variable is a non-const value and whether only + // const methods are invoked on it or whether it is only used as a const + // reference argument. If so it suggests it be made a const reference. + bool handleCopyIsOnlyConstReferenced(const VarDecl &LoopVar, + const CXXForRangeStmt &ForRange, + ASTContext &Context); + + const bool WarnOnAllAutoCopies; +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FORRANGECOPYCHECK_H Index: clang-tidy/performance/ForRangeCopyCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/performance/ForRangeCopyCheck.cpp @@ -0,0 +1,170 @@ +//===--- ForRangeCopyCheck.cpp - clang-tidy--------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "ForRangeCopyCheck.h" +#include "../utils/LexerUtils.h" +#include "../utils/TypeTraits.h" +#include "clang/Lex/Lexer.h" + +#include + +namespace clang { +namespace tidy { +namespace performance { + +using namespace ::clang::ast_matchers; + +namespace { + +template bool isSetDifferenceEmpty(const S &S1, const S &S2) { + for (const auto &E : S1) + if (S2.find(E) == S2.end()) + return false; + return true; +} + +// Extracts all Nodes keyed by ID from Matches and inserts them into Nodes. +template +void extractNodesByIdTo(const SmallVector &Matches, StringRef ID, + std::set &Nodes) { + for (const auto &Match : Matches) + Nodes.insert(Match.getNodeAs(ID)); +} + +// Finds all DeclRefExprs to VarDecl in Stmt. +std::set +declRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) { + auto Matches = match( + findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")), + Stmt, Context); + std::set DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} + +// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl +// is the a const reference or value argument to a CallExpr or CXXConstructExpr. +std::set constReferenceDeclRefExprs(const VarDecl &VarDecl, + const Stmt &Stmt, + ASTContext &Context) { + auto DeclRefToVar = + declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"); + auto ConstMethodCallee = callee(cxxMethodDecl(isConst())); + // Match method call expressions where the variable is referenced as the this + // implicit object argument and opertor call expression for member operators + // where the variable is the 0-th argument. + auto Matches = match( + findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)), + cxxOperatorCallExpr(ConstMethodCallee, + hasArgument(0, DeclRefToVar))))), + Stmt, Context); + std::set DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + auto ConstReferenceOrValue = + qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))), + unless(anyOf(referenceType(), pointerType())))); + auto UsedAsConstRefOrValueArg = forEachArgumentWithParam( + DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue))); + Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context); + extractNodesByIdTo(Matches, "declRef", DeclRefs); + Matches = + match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context); + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} + +// Modifies VarDecl to be a reference. +FixItHint createAmpersandFix(const VarDecl &VarDecl, ASTContext &Context) { + SourceLocation AmpLocation = VarDecl.getLocation(); + auto Token = lexer_utils::getPreviousNonCommentToken(Context, AmpLocation); + if (!Token.is(tok::unknown)) + AmpLocation = Token.getLocation().getLocWithOffset(Token.getLength()); + return FixItHint::CreateInsertion(AmpLocation, "&"); +} + +// Modifies VarDecl to be const. +FixItHint createConstFix(const VarDecl &VarDecl) { + return FixItHint::CreateInsertion(VarDecl.getTypeSpecStartLoc(), "const "); +} + +} // namespace + +ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)) {} + +void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies); +} + +void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) { + auto LoopVar = + varDecl(hasType(qualType(unless(anyOf(referenceType(), pointerType()))))); + Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar"))) + .bind("forRange"), + this); +} + +void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Var = Result.Nodes.getNodeAs("loopVar"); + const auto *ForRange = Result.Nodes.getNodeAs("forRange"); + if (handleConstValueCopy(*Var, *Result.Context)) + return; + handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context); +} + +bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar, + ASTContext &Context) { + if (WarnOnAllAutoCopies) { + // For aggressive check just test that loop variable has auto type. + if (!isa(LoopVar.getType())) + return false; + } else if (!LoopVar.getType().isConstQualified()) { + return false; + } + if (!type_traits::isExpensiveToCopy(LoopVar.getType(), Context)) + return false; + auto Diagnostic = + diag(LoopVar.getLocation(), + "the loop variable's type is not a reference type; this creates a " + "copy in each iteration; consider making this a reference") + << createAmpersandFix(LoopVar, Context); + if (!LoopVar.getType().isConstQualified()) + Diagnostic << createConstFix(LoopVar); + return true; +} + +bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( + const VarDecl &LoopVar, const CXXForRangeStmt &ForRange, + ASTContext &Context) { + if (LoopVar.getType().isConstQualified() || + !type_traits::isExpensiveToCopy(LoopVar.getType(), Context)) { + return false; + } + // Collect all DeclRefExprs to the loop variable and all CallExprs and + // CXXConstructExprs where the loop variable is used as argument to a const + // reference parameter. + // If the difference is empty it is safe for the loop variable to be a const + // reference. + auto AllDeclRefs = declRefExprs(LoopVar, *ForRange.getBody(), Context); + auto ConstReferenceDeclRefs = + constReferenceDeclRefExprs(LoopVar, *ForRange.getBody(), Context); + if (AllDeclRefs.empty() || + !isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs)) + return false; + diag(LoopVar.getLocation(), + "loop variable is copied but only used as const reference; consider " + "making it a const reference") + << createConstFix(LoopVar) << createAmpersandFix(LoopVar, Context); + return true; +} + +} // namespace performance +} // namespace tidy +} // namespace clang Index: clang-tidy/performance/PerformanceTidyModule.cpp =================================================================== --- clang-tidy/performance/PerformanceTidyModule.cpp +++ clang-tidy/performance/PerformanceTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ForRangeCopyCheck.h" #include "UnnecessaryCopyInitialization.h" namespace clang { @@ -20,6 +21,7 @@ class PerformanceModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck("performance-for-range-copy"); CheckFactories.registerCheck( "performance-unnecessary-copy-initialization"); } Index: test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp =================================================================== --- /dev/null +++ test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp @@ -0,0 +1,36 @@ +// RUN: %check_clang_tidy %s performance-for-range-copy %t -config="{CheckOptions: [{key: "performance-for-range-copy.WarnOnAllAutoCopies", value: 1}]}" -- -std=c++11 + +template +struct Iterator { + void operator++() {} + T operator*() { return T(); } + bool operator!=(const Iterator &) { return false; } +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } +}; + +struct S { + S(); + S(const S &); + ~S(); + S &operator=(const S &); +}; + +void NegativeLoopVariableNotAuto() { + for (S S1 : View>()) { + S* S2 = &S1; + } +} + +void PositiveTriggeredForAutoLoopVariable() { + for (auto S1 : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& S1 : View>()) { + S* S2 = &S1; + } +} Index: test/clang-tidy/performance-for-range-copy.cpp =================================================================== --- /dev/null +++ test/clang-tidy/performance-for-range-copy.cpp @@ -0,0 +1,188 @@ +// RUN: %check_clang_tidy %s performance-for-range-copy %t + +namespace std { + +template +struct remove_reference { typedef _Tp type; }; + +template +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) { + return static_cast::type &&>(__t); +} + +} // std + +template +struct Iterator { + void operator++() {} + T operator*() { return T(); } + bool operator!=(const Iterator &) { return false; } +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } +}; + +struct S { + S(); + S(const S &); + ~S(); + S &operator=(const S &); +}; + +void NegativeConstReference() { + for (const S &S1 : View>()) { + } +} + +template +void uninstantiated() { + for (const S S1 : View>()) {} + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy] + // CHECK-FIXES: {{^}} for (const S& S1 : View>()) {} + + // Don't warn on dependent types. + for (const T t1 : View>()) { + } +} + +template +void instantiated() { + for (const S S2 : View>()) {} + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}} + // CHECK-FIXES: {{^}} for (const S& S2 : View>()) {} + + for (const T T2 : View>()) {} + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}} + // CHECK-FIXES: {{^}} for (const T& T2 : View>()) {} +} + +void f() { + instantiated(); + instantiated(); +} + +struct Mutable { + Mutable() {} + Mutable(const Mutable &) = default; + Mutable(const Mutable &, const Mutable &) {} + void setBool(bool B) {} + bool constMethod() const { + return true; + } + Mutable& operator[](int I) { + return *this; + } + bool operator==(const Mutable &Other) const { + return true; + } + ~Mutable() {} +}; + +Mutable& operator<<(Mutable &Out, bool B) { + Out.setBool(B); + return Out; +} + +bool operator!=(const Mutable& M1, const Mutable& M2) { + return false; +} + +void use(const Mutable &M); +void useTwice(const Mutable &M1, const Mutable &M2); +void useByValue(Mutable M); +void useByConstValue(const Mutable M); +void mutate(Mutable *M); +void mutate(Mutable &M); +void onceConstOnceMutated(const Mutable &M1, Mutable &M2); + +void negativeVariableIsMutated() { + for (auto M : View>()) { + mutate(M); + } + for (auto M : View>()) { + mutate(&M); + } + for (auto M : View>()) { + M.setBool(true); + } +} + +void negativeOnceConstOnceMutated() { + for (auto M : View>()) { + onceConstOnceMutated(M, M); + } +} + +void negativeVarIsMoved() { + for (auto M : View>()) { + auto Moved = std::move(M); + } +} + +void negativeNonConstOperatorIsInvoked() { + for (auto NonConstOperatorInvokee : View>()) { + auto& N = NonConstOperatorInvokee[0]; + } +} + +void negativeNonConstNonMemberOperatorInvoked() { + for (auto NonConstOperatorInvokee : View>()) { + NonConstOperatorInvokee << true; + } +} + +void positiveOnlyConstMethodInvoked() { + for (auto M : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& M : View>()) { + M.constMethod(); + } +} + +void positiveOnlyUsedAsConstArguments() { + for (auto UsedAsConst : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& UsedAsConst : View>()) { + use(UsedAsConst); + useTwice(UsedAsConst, UsedAsConst); + useByValue(UsedAsConst); + useByConstValue(UsedAsConst); + } +} + +void positiveOnlyUsedInCopyConstructor() { + for (auto A : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& A : View>()) { + Mutable Copy = A; + Mutable Copy2(A); + } +} + +void positiveTwoConstConstructorArgs() { + for (auto A : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& A : View>()) { + Mutable Copy(A, A); + } +} + +void PositiveConstMemberOperatorInvoked() { + for (auto ConstOperatorInvokee : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& ConstOperatorInvokee : View>()) { + bool result = ConstOperatorInvokee == Mutable(); + } +} + +void PositiveConstNonMemberOperatorInvoked() { + for (auto ConstOperatorInvokee : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& ConstOperatorInvokee : View>()) { + bool result = ConstOperatorInvokee != Mutable(); + } +}