Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[clang-tidy] ForRangeCopyCheck that warns on and fixes unnecessary co…
…pies of loop variables. Patch by Felix Berger! Differential revision: http://reviews.llvm.org/D13849 llvm-svn: 259199
- Loading branch information
Showing
9 changed files
with
525 additions
and
5 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
177 changes: 177 additions & 0 deletions
177
clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 <typename S> 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 <typename Node> | ||
void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, | ||
SmallPtrSet<const Node *, 16> &Nodes) { | ||
for (const auto &Match : Matches) | ||
Nodes.insert(Match.getNodeAs<Node>(ID)); | ||
} | ||
|
||
// Finds all DeclRefExprs to VarDecl in Stmt. | ||
SmallPtrSet<const DeclRefExpr *, 16> | ||
declRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) { | ||
auto Matches = match( | ||
findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")), | ||
Stmt, Context); | ||
SmallPtrSet<const DeclRefExpr *, 16> 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<const DeclRefExpr *, 16> | ||
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<const DeclRefExpr *, 16> 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<VarDecl>("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<CXXForRangeStmt>("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<AutoType>(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 |
49 changes: 49 additions & 0 deletions
49
clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
19 changes: 19 additions & 0 deletions
19
clang-tools-extra/docs/clang-tidy/checks/performance-for-range-copy.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. |
39 changes: 39 additions & 0 deletions
39
clang-tools-extra/test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 <typename T> | ||
struct Iterator { | ||
void operator++() {} | ||
const T& operator*() { | ||
static T* TT = new T(); | ||
return *TT; | ||
} | ||
bool operator!=(const Iterator &) { return false; } | ||
}; | ||
template <typename T> | ||
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<Iterator<S>>()) { | ||
S* S2 = &S1; | ||
} | ||
} | ||
|
||
void PositiveTriggeredForAutoLoopVariable() { | ||
for (auto S1 : View<Iterator<S>>()) { | ||
// 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<Iterator<S>>()) { | ||
S* S2 = &S1; | ||
} | ||
} |
Oops, something went wrong.