diff --git a/clang-tools-extra/clang-tidy/add_new_check.py b/clang-tools-extra/clang-tidy/add_new_check.py index 6000616d0b09..a037d1f3069a 100755 --- a/clang-tools-extra/clang-tidy/add_new_check.py +++ b/clang-tools-extra/clang-tidy/add_new_check.py @@ -214,8 +214,8 @@ void awesome_f2(); """ % {"check_name_dashes" : check_name_dashes}) # Recreates the list of checks in the docs/clang-tidy/checks directory. -def update_checks_list(module_path): - docs_dir = os.path.join(module_path, '../../docs/clang-tidy/checks') +def update_checks_list(clang_tidy_path): + docs_dir = os.path.join(clang_tidy_path, '../docs/clang-tidy/checks') filename = os.path.normpath(os.path.join(docs_dir, 'list.rst')) with open(filename, 'r') as f: lines = f.readlines() @@ -262,9 +262,17 @@ FIXME: Describe what patterns does the check detect and why. Give examples. "underline" : "=" * len(check_name_dashes)}) def main(): + if len(sys.argv) == 2 and sys.argv[1] == '--update-docs': + update_checks_list(os.path.dirname(sys.argv[0])) + return + if len(sys.argv) != 3: - print 'Usage: add_new_check.py , e.g.\n' - print 'add_new_check.py misc awesome-functions\n' + print """\ +Usage: add_new_check.py , e.g. + add_new_check.py misc awesome-functions + +Alternatively, run 'add_new_check.py --update-docs' to just update the list of +documentation files.""" return module = sys.argv[1] @@ -281,7 +289,7 @@ def main(): adapt_module(module_path, module, check_name, check_name_camel) write_test(module_path, module, check_name) write_docs(module_path, module, check_name) - update_checks_list(module_path) + update_checks_list(clang_tidy_path) print('Done. Now it\'s your turn!') if __name__ == '__main__': diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index 322b00079432..66a5127cc846 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -1,6 +1,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyPerformanceModule + ForRangeCopyCheck.cpp ImplicitCastInLoopCheck.cpp PerformanceTidyModule.cpp UnnecessaryCopyInitialization.cpp diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp new file mode 100644 index 000000000000..88e277720fb9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp @@ -0,0 +1,177 @@ +//===--- 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 "llvm/ADT/SmallPtrSet.h" + +namespace clang { +namespace tidy { +namespace performance { + +using namespace ::clang::ast_matchers; +using llvm::SmallPtrSet; + +namespace { + +template bool isSetDifferenceEmpty(const S &S1, const S &S2) { + for (const auto &E : S1) + if (S2.count(E) == 0) + return false; + return true; +} + +// Extracts all Nodes keyed by ID from Matches and inserts them into Nodes. +template +void extractNodesByIdTo(ArrayRef Matches, StringRef ID, + SmallPtrSet &Nodes) { + for (const auto &Match : Matches) + Nodes.insert(Match.getNodeAs(ID)); +} + +// Finds all DeclRefExprs to VarDecl in Stmt. +SmallPtrSet +declRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) { + auto Matches = match( + findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")), + Stmt, Context); + SmallPtrSet 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. +SmallPtrSet +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); + SmallPtrSet 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) { + // Match loop variables that are not references or pointers or are already + // initialized through MaterializeTemporaryExpr which indicates a type + // conversion. + auto LoopVar = varDecl( + hasType(hasCanonicalType(unless(anyOf(referenceType(), pointerType())))), + unless(hasInitializer(expr(hasDescendant(materializeTemporaryExpr()))))); + Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar"))) + .bind("forRange"), + this); +} + +void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Var = Result.Nodes.getNodeAs("loopVar"); + // Ignore code in macros since we can't place the fixes correctly. + if (Var->getLocStart().isMacroID()) + return; + if (handleConstValueCopy(*Var, *Result.Context)) + return; + const auto *ForRange = Result.Nodes.getNodeAs("forRange"); + 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 diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h new file mode 100644 index 000000000000..f88a126e5d75 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h @@ -0,0 +1,49 @@ +//===--- 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. +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance-for-range-copy.html +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 diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index 977e6a86b646..32097eb62497 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ForRangeCopyCheck.h" #include "ImplicitCastInLoopCheck.h" #include "UnnecessaryCopyInitialization.h" @@ -21,6 +22,8 @@ namespace performance { class PerformanceModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "performance-for-range-copy"); CheckFactories.registerCheck( "performance-implicit-cast-in-loop"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 42d8c103782d..c2bd248d1043 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -76,6 +76,7 @@ Clang-Tidy Checks modernize-use-default modernize-use-nullptr modernize-use-override + performance-for-range-copy performance-implicit-cast-in-loop readability-braces-around-statements readability-container-size-empty diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst new file mode 100644 index 000000000000..a3af055f7b69 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst @@ -0,0 +1,19 @@ +.. title:: clang-tidy - performance-for-range-copy + +performance-for-range-copy +========================== + +Finds C++11 for ranges where the loop variable is copied in each iteration but +it would suffice to obtain it by const reference. + +The check is only applied to loop variables of types that are expensive to copy +which means they are not trivially copyable or have a non-trivial copy +constructor or destructor. + +To ensure that it is safe to replace the copy with const reference the following +heuristic is employed: + +1. The loop variable is const qualified. +2. The loop variable is not const, but only const methods or operators are + invoked on it, or it is used as const reference or value argument in + constructors or function calls. diff --git a/clang-tools-extra/test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp b/clang-tools-extra/test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp new file mode 100644 index 000000000000..75655a6fffb7 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp @@ -0,0 +1,39 @@ +// 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++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + 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; + } +} diff --git a/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp b/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp new file mode 100644 index 000000000000..c65160936a7e --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp @@ -0,0 +1,223 @@ +// 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++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + bool operator!=(const Iterator &) { return false; } + typedef const T& const_reference; +}; +template +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } + typedef typename T::const_reference const_reference; +}; + +struct ConstructorConvertible { +}; + +struct S { + S(); + S(const S &); + S(const ConstructorConvertible&) {} + ~S(); + S &operator=(const S &); +}; + +struct Convertible { + operator S() const { + return S(); + } +}; + +void negativeConstReference() { + for (const S &S1 : View>()) { + } +} + +void negativeUserDefinedConversion() { + Convertible C[0]; + for (const S &S1 : C) { + } +} + +void negativeImplicitConstructorConversion() { + ConstructorConvertible C[0]; + for (const S &S1 : C) { + } +} + +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>()) {} +} + +template +void instantiatedNegativeTypedefConstReference() { + for (typename T::const_reference T2 : T()) { + S S1 = T2; + } +} + +void f() { + instantiated(); + instantiated(); + instantiatedNegativeTypedefConstReference>>(); +} + +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(); + } +}