maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

Repair various issues with modernize-avoid-bind 

In the process of running this check on a large codebase I found a
number of limitations, and thought I would pass on my fixes for
possible integration upstream:

* Templated function call operators are not supported
* Function object constructors are always used directly in the lambda
  body, even if their arguments are not captured
* Placeholders with namespace qualifiers (std::placeholders::_1) are
  not detected
* Lambda arguments should be forwarded to the stored function
* Data members from other classes still get captured with this
* Expressions (as opposed to variables) inside std::ref are not captured
  properly
* Function object templates sometimes have their template arguments
  replaced with concrete types

This patch resolves all those issues and adds suitable unit tests.
This commit is contained in:
Jeff Trull 2020-06-25 07:29:53 -04:00 committed by Aaron Ballman
parent c95ffadb24
commit 95a3550dc8
3 changed files with 154 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

120
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
View File

@ -35,7 +35,8 @@ namespace modernize {
namespace {
enum BindArgumentKind { BK_Temporary, BK_Placeholder, BK_CallExpr, BK_Other };
enum CaptureMode { CM_None, CM_ByRef, CM_ByValue, CM_InitExpression };
enum CaptureMode { CM_None, CM_ByRef, CM_ByValue };
enum CaptureExpr { CE_None, CE_Var, CE_InitExpression };
enum CallableType {
CT_Other, // unknown
@ -60,6 +61,10 @@ struct BindArgument {
// captured.
CaptureMode CM = CM_None;
// Whether the argument is a simple variable (we can capture it directly),
// or an expression (we must introduce a capture variable).
CaptureExpr CE = CE_None;
// The exact spelling of this argument in the source code.
StringRef SourceTokens;
@ -86,6 +91,7 @@ struct CallableInfo {
CallableType Type = CT_Other;
CallableMaterializationKind Materialization = CMK_Other;
CaptureMode CM = CM_None;
CaptureExpr CE = CE_None;
StringRef SourceTokens;
std::string CaptureIdentifier;
std::string UsageIdentifier;
@ -102,6 +108,12 @@ struct LambdaProperties {
} // end namespace
static bool tryCaptureAsLocalVariable(const MatchFinder::MatchResult &Result,
BindArgument &B, const Expr *E);
static bool tryCaptureAsMemberVariable(const MatchFinder::MatchResult &Result,
BindArgument &B, const Expr *E);
static const Expr *ignoreTemporariesAndPointers(const Expr *E) {
if (const auto *T = dyn_cast<UnaryOperator>(E))
return ignoreTemporariesAndPointers(T->getSubExpr());
@ -148,12 +160,22 @@ initializeBindArgumentForCallExpr(const MatchFinder::MatchResult &Result,
// std::ref(x) means to capture x by reference.
if (isCallExprNamed(CE, "boost::ref") || isCallExprNamed(CE, "std::ref")) {
B.Kind = BK_Other;
if (tryCaptureAsLocalVariable(Result, B, CE->getArg(0)) ||
tryCaptureAsMemberVariable(Result, B, CE->getArg(0))) {
B.CE = CE_Var;
} else {
// The argument to std::ref is an expression that produces a reference.
// Create a capture reference to hold it.
B.CE = CE_InitExpression;
B.UsageIdentifier = "capture" + llvm::utostr(CaptureIndex++);
}
// Strip off the reference wrapper.
B.SourceTokens = getSourceTextForExpr(Result, CE->getArg(0));
B.CM = CM_ByRef;
B.UsageIdentifier =
std::string(getSourceTextForExpr(Result, CE->getArg(0)));
} else {
B.Kind = BK_CallExpr;
B.CM = CM_InitExpression;
B.CM = CM_ByValue;
B.CE = CE_InitExpression;
B.UsageIdentifier = "capture" + llvm::utostr(CaptureIndex++);
}
B.CaptureIdentifier = B.UsageIdentifier;
@ -204,6 +226,7 @@ static bool tryCaptureAsMemberVariable(const MatchFinder::MatchResult &Result,
E = E->IgnoreImplicit();
if (isa<CXXThisExpr>(E)) {
// E is a direct use of "this".
B.CM = CM_ByValue;
B.UsageIdentifier = std::string(getSourceTextForExpr(Result, E));
B.CaptureIdentifier = "this";
@ -217,10 +240,15 @@ static bool tryCaptureAsMemberVariable(const MatchFinder::MatchResult &Result,
if (!ME->isLValue() || !isa<FieldDecl>(ME->getMemberDecl()))
return false;
B.CM = CM_ByValue;
B.UsageIdentifier = std::string(getSourceTextForExpr(Result, E));
B.CaptureIdentifier = "this";
return true;
if (isa<CXXThisExpr>(ME->getBase())) {
// E refers to a data member without an explicit "this".
B.CM = CM_ByValue;
B.UsageIdentifier = std::string(getSourceTextForExpr(Result, E));
B.CaptureIdentifier = "this";
return true;
}
return false;
}
static SmallVector<BindArgument, 4>
@ -251,7 +279,10 @@ buildBindArguments(const MatchFinder::MatchResult &Result,
B.IsUsed = true;
SmallVector<StringRef, 2> Matches;
if (MatchPlaceholder.match(B.SourceTokens, &Matches)) {
const auto *DRE = dyn_cast<DeclRefExpr>(E);
if (MatchPlaceholder.match(B.SourceTokens, &Matches) ||
// Check for match with qualifiers removed.
(DRE && MatchPlaceholder.match(DRE->getDecl()->getName(), &Matches))) {
B.Kind = BK_Placeholder;
B.PlaceHolderIndex = std::stoi(std::string(Matches[1]));
B.UsageIdentifier = "PH" + llvm::utostr(B.PlaceHolderIndex);
@ -273,11 +304,13 @@ buildBindArguments(const MatchFinder::MatchResult &Result,
// safe.
B.Kind = BK_Other;
if (IsObjectPtr) {
B.CM = CM_InitExpression;
B.CE = CE_InitExpression;
B.CM = CM_ByValue;
B.UsageIdentifier = "ObjectPtr";
B.CaptureIdentifier = B.UsageIdentifier;
} else if (anyDescendantIsLocal(B.E)) {
B.CM = CM_InitExpression;
B.CE = CE_InitExpression;
B.CM = CM_ByValue;
B.CaptureIdentifier = "capture" + llvm::utostr(CaptureIndex++);
B.UsageIdentifier = B.CaptureIdentifier;
}
@ -337,9 +370,12 @@ static void addFunctionCallArgs(ArrayRef<BindArgument> Args,
Stream << Delimiter;
if (B.Kind == BK_Placeholder || B.CM != CM_None)
if (B.Kind == BK_Placeholder) {
Stream << "std::forward<decltype(" << B.UsageIdentifier << ")>";
Stream << "(" << B.UsageIdentifier << ")";
} else if (B.CM != CM_None)
Stream << B.UsageIdentifier;
else if (B.CM == CM_None)
else
Stream << B.SourceTokens;
Delimiter = ", ";
@ -357,9 +393,9 @@ static bool isPlaceHolderIndexRepeated(const ArrayRef<BindArgument> Args) {
return false;
}
static std::vector<const CXXMethodDecl *>
static std::vector<const FunctionDecl *>
findCandidateCallOperators(const CXXRecordDecl *RecordDecl, size_t NumArgs) {
std::vector<const CXXMethodDecl *> Candidates;
std::vector<const FunctionDecl *> Candidates;
for (const clang::CXXMethodDecl *Method : RecordDecl->methods()) {
OverloadedOperatorKind OOK = Method->getOverloadedOperator();
@ -373,6 +409,23 @@ findCandidateCallOperators(const CXXRecordDecl *RecordDecl, size_t NumArgs) {
Candidates.push_back(Method);
}
// Find templated operator(), if any.
for (const clang::Decl *D : RecordDecl->decls()) {
const auto *FTD = dyn_cast<FunctionTemplateDecl>(D);
if (!FTD)
continue;
const FunctionDecl *FD = FTD->getTemplatedDecl();
OverloadedOperatorKind OOK = FD->getOverloadedOperator();
if (OOK != OverloadedOperatorKind::OO_Call)
continue;
if (FD->getNumParams() > NumArgs)
continue;
Candidates.push_back(FD);
}
return Candidates;
}
@ -407,7 +460,7 @@ static bool isFixitSupported(const CallableInfo &Callee,
const FunctionDecl *getCallOperator(const CXXRecordDecl *Callable,
size_t NumArgs) {
std::vector<const CXXMethodDecl *> Candidates =
std::vector<const FunctionDecl *> Candidates =
findCandidateCallOperators(Callable, NumArgs);
if (Candidates.size() != 1)
return nullptr;
@ -466,11 +519,15 @@ getCallableMaterialization(const MatchFinder::MatchResult &Result) {
const auto *NoTemporaries = ignoreTemporariesAndPointers(CallableExpr);
if (isa<CallExpr>(NoTemporaries))
const auto *CE = dyn_cast<CXXConstructExpr>(NoTemporaries);
const auto *FC = dyn_cast<CXXFunctionalCastExpr>(NoTemporaries);
if ((isa<CallExpr>(NoTemporaries)) || (CE && (CE->getNumArgs() > 0)) ||
(FC && (FC->getCastKind() == CK_ConstructorConversion)))
// CE is something that looks like a call, with arguments - either
// a function call or a constructor invocation.
return CMK_CallExpression;
if (isa<CXXFunctionalCastExpr>(NoTemporaries) ||
isa<CXXConstructExpr>(NoTemporaries))
if (isa<CXXFunctionalCastExpr>(NoTemporaries) || CE)
return CMK_Function;
if (const auto *DRE = dyn_cast<DeclRefExpr>(NoTemporaries)) {
@ -503,13 +560,15 @@ getLambdaProperties(const MatchFinder::MatchResult &Result) {
getCallMethodDecl(Result, LP.Callable.Type, LP.Callable.Materialization);
LP.Callable.SourceTokens = getSourceTextForExpr(Result, CalleeExpr);
if (LP.Callable.Materialization == CMK_VariableRef) {
LP.Callable.CE = CE_Var;
LP.Callable.CM = CM_ByValue;
LP.Callable.UsageIdentifier =
std::string(getSourceTextForExpr(Result, CalleeExpr));
LP.Callable.CaptureIdentifier = std::string(
getSourceTextForExpr(Result, ignoreTemporariesAndPointers(CalleeExpr)));
} else if (LP.Callable.Materialization == CMK_CallExpression) {
LP.Callable.CM = CM_InitExpression;
LP.Callable.CE = CE_InitExpression;
LP.Callable.CM = CM_ByValue;
LP.Callable.UsageIdentifier = "Func";
LP.Callable.CaptureIdentifier = "Func";
LP.Callable.CaptureInitializer = getSourceTextForExpr(Result, CalleeExpr);
@ -523,7 +582,7 @@ getLambdaProperties(const MatchFinder::MatchResult &Result) {
}
static bool emitCapture(llvm::StringSet<> &CaptureSet, StringRef Delimiter,
CaptureMode CM, StringRef Identifier,
CaptureMode CM, CaptureExpr CE, StringRef Identifier,
StringRef InitExpression, raw_ostream &Stream) {
if (CM == CM_None)
return false;
@ -537,7 +596,7 @@ static bool emitCapture(llvm::StringSet<> &CaptureSet, StringRef Delimiter,
if (CM == CM_ByRef)
Stream << "&";
Stream << Identifier;
if (CM == CM_InitExpression)
if (CE == CE_InitExpression)
Stream << " = " << InitExpression;
CaptureSet.insert(Identifier);
@ -550,9 +609,9 @@ static void emitCaptureList(const LambdaProperties &LP,
llvm::StringSet<> CaptureSet;
bool AnyCapturesEmitted = false;
AnyCapturesEmitted = emitCapture(CaptureSet, "", LP.Callable.CM,
LP.Callable.CaptureIdentifier,
LP.Callable.CaptureInitializer, Stream);
AnyCapturesEmitted = emitCapture(
CaptureSet, "", LP.Callable.CM, LP.Callable.CE,
LP.Callable.CaptureIdentifier, LP.Callable.CaptureInitializer, Stream);
for (const BindArgument &B : LP.BindArguments) {
if (B.CM == CM_None || !B.IsUsed)
@ -560,7 +619,7 @@ static void emitCaptureList(const LambdaProperties &LP,
StringRef Delimiter = AnyCapturesEmitted ? ", " : "";
if (emitCapture(CaptureSet, Delimiter, B.CM, B.CaptureIdentifier,
if (emitCapture(CaptureSet, Delimiter, B.CM, B.CE, B.CaptureIdentifier,
B.SourceTokens, Stream))
AnyCapturesEmitted = true;
}
@ -635,19 +694,18 @@ void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
Stream << MethodDecl->getName();
} else {
Stream << " { return ";
switch (LP.Callable.CM) {
case CM_ByValue:
case CM_ByRef:
switch (LP.Callable.CE) {
case CE_Var:
if (LP.Callable.UsageIdentifier != LP.Callable.CaptureIdentifier) {
Stream << "(" << LP.Callable.UsageIdentifier << ")";
break;
}
LLVM_FALLTHROUGH;
case CM_InitExpression:
case CE_InitExpression:
Stream << LP.Callable.UsageIdentifier;
break;
default:
Ref->printPretty(Stream, nullptr, Result.Context->getPrintingPolicy());
Stream << getSourceTextForExpr(Result, Ref);
}
}

2
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
View File

@ -54,5 +54,5 @@ void testLiteralParameters() {
auto BBB = std::bind(add, _1, 2);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
// CHECK-FIXES: auto BBB = [](auto && PH1, auto && ...) { return add(PH1, 2); };
// CHECK-FIXES: auto BBB = [](auto && PH1, auto && ...) { return add(std::forward<decltype(PH1)>(PH1), 2); };
}

69
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
View File

@ -7,11 +7,11 @@ class bind_rt {};
template <class Fp, class... Arguments>
bind_rt<Fp, Arguments...> bind(Fp &&, Arguments &&...);
}
} // namespace impl
template <typename T>
T ref(T &t);
}
} // namespace std
namespace boost {
template <class Fp, class... Arguments>
@ -58,12 +58,33 @@ struct F {
void UseF(F);
struct G {
G() : _member(0) {}
G(int m) : _member(m) {}
template <typename T>
void operator()(T) const {}
int _member;
};
template <typename T>
struct H {
void operator()(T) const {};
};
struct placeholder {};
placeholder _1;
placeholder _2;
namespace placeholders {
using ::_1;
using ::_2;
} // namespace placeholders
int add(int x, int y) { return x + y; }
int addThree(int x, int y, int z) { return x + y + z; }
void sub(int &x, int y) { x += y; }
// Let's fake a minimal std::function-like facility.
namespace std {
@ -107,6 +128,7 @@ struct TestCaptureByValueStruct {
int MemberVariable;
static int StaticMemberVariable;
F MemberStruct;
G MemberStructWithData;
void testCaptureByValue(int Param, F f) {
int x = 3;
@ -145,6 +167,11 @@ struct TestCaptureByValueStruct {
auto GGG = boost::bind(UseF, MemberStruct);
// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind]
// CHECK-FIXES: auto GGG = [this] { return UseF(MemberStruct); };
auto HHH = std::bind(add, MemberStructWithData._member, 1);
// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
// Correctly distinguish data members of other classes
// CHECK-FIXES: auto HHH = [capture0 = MemberStructWithData._member] { return add(capture0, 1); };
}
};
@ -217,17 +244,38 @@ void testFunctionObjects() {
auto EEE = std::bind(*D::create(), 1, 2);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// CHECK-FIXES: auto EEE = [Func = *D::create()] { return Func(1, 2); };
auto FFF = std::bind(G(), 1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// Templated function call operators may be used
// CHECK-FIXES: auto FFF = [] { return G()(1); };
int CTorArg = 42;
auto GGG = std::bind(G(CTorArg), 1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// Function objects with constructor arguments should be captured
// CHECK-FIXES: auto GGG = [Func = G(CTorArg)] { return Func(1); };
}
template <typename T>
void testMemberFnOfClassTemplate(T) {
auto HHH = std::bind(H<T>(), 42);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// Ensure function class template arguments are preserved
// CHECK-FIXES: auto HHH = [] { return H<T>()(42); };
}
template void testMemberFnOfClassTemplate(int);
void testPlaceholders() {
int x = 2;
auto AAA = std::bind(add, x, _1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, PH1); };
// CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, std::forward<decltype(PH1)>(PH1)); };
auto BBB = std::bind(add, _2, _1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(PH2, PH1); };
// CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(std::forward<decltype(PH2)>(PH2), std::forward<decltype(PH1)>(PH1)); };
// No fix is applied for reused placeholders.
auto CCC = std::bind(add, _1, _1);
@ -238,7 +286,12 @@ void testPlaceholders() {
// unnamed parameters.
auto DDD = std::bind(add, _2, 1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(PH2, 1); };
// CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(std::forward<decltype(PH2)>(PH2), 1); };
// Namespace-qualified placeholders are valid too
auto EEE = std::bind(add, placeholders::_2, 1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// CHECK-FIXES: auto EEE = [](auto &&, auto && PH2) { return add(std::forward<decltype(PH2)>(PH2), 1); };
}
void testGlobalFunctions() {
@ -267,6 +320,7 @@ void testGlobalFunctions() {
void testCapturedSubexpressions() {
int x = 3;
int y = 3;
int *p = &x;
auto AAA = std::bind(add, 1, add(2, 5));
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
@ -277,6 +331,11 @@ void testCapturedSubexpressions() {
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// Results of nested calls are captured by value.
// CHECK-FIXES: auto BBB = [x, capture0 = add(y, 5)] { return add(x, capture0); };
auto CCC = std::bind(sub, std::ref(*p), _1);
// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
// Expressions returning references are captured
// CHECK-FIXES: auto CCC = [&capture0 = *p](auto && PH1) { return sub(capture0, std::forward<decltype(PH1)>(PH1)); };
}
struct E {