[clang-tools-extra] 95a3550 - Repair various issues with modernize-avoid-bind
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 25 04:32:41 PDT 2020
Author: Jeff Trull
Date: 2020-06-25T07:29:53-04:00
New Revision: 95a3550dc89a0d424d90e2c0ad30d9ecfa9422cf
URL: https://github.com/llvm/llvm-project/commit/95a3550dc89a0d424d90e2c0ad30d9ecfa9422cf
DIFF: https://github.com/llvm/llvm-project/commit/95a3550dc89a0d424d90e2c0ad30d9ecfa9422cf.diff
LOG: 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.
Added:
Modified:
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
index 37f9eb6955d3..cadb84e1d038 100644
--- a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
@@ -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);
}
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
index 6c81a6e9ab97..e055fd323230 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
@@ -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); };
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
index 7e00858c1acc..3334eab2d407 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -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 {
More information about the cfe-commits
mailing list