jaafar created this revision.
jaafar added reviewers: aaron.ballman, alexfh, hokein.
jaafar added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
In the process of running this check on a large codebase
<https://git.skewed.de/count0/graph-tool> I found a number of limitations, and
thought I would pass on my fixes for possible integration upstream:
1. Templated function call operators are not supported
2. Function object constructors are always used directly in the lambda body,
even if their arguments are not captured
3. Placeholders with namespace qualifiers (`std::placeholders::_1`) are not
detected
4. Lambda arguments should be forwarded to the stored function
5. Data members from other classes still get captured with `this`
6. Expressions (as opposed to variables) inside `std::ref` are not captured
properly
7. Function object templates sometimes have their template arguments replaced
with concrete types
This patch resolves all those issues and adds suitable unit tests.
If desired, I can separate these commits out into 7 separate diffs, but it
seemed like it might be easier to evaluate them all at once.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D81769
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -7,11 +7,11 @@
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 @@
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 @@
int MemberVariable;
static int StaticMemberVariable;
F MemberStruct;
+ G MemberStructWithData;
void testCaptureByValue(int Param, F f) {
int x = 3;
@@ -145,6 +167,11 @@
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 @@
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 @@
// 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 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 @@
// 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 {
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.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-permissive-parameter-list.cpp
@@ -54,5 +54,5 @@
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); };
}
Index: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
@@ -35,7 +35,8 @@
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 @@
// captured.
CaptureMode CM = CM_None;
+ // Whether the argument is a simple variable (we can capture it directly)
+ // or an expression (we introduce a capture variable)
+ CaptureExpr CE = CE_None;
+
// The exact spelling of this argument in the source code.
StringRef SourceTokens;
@@ -86,6 +91,7 @@
CallableType Type = CT_Other;
CallableMaterializationKind Materialization = CMK_Other;
CaptureMode CM = CM_None;
+ CaptureExpr CE = CE_None;
StringRef SourceTokens;
std::string CaptureIdentifier;
std::string UsageIdentifier;
@@ -141,6 +147,11 @@
return ND->getQualifiedNameAsString() == Name;
}
+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 void
initializeBindArgumentForCallExpr(const MatchFinder::MatchResult &Result,
BindArgument &B, const CallExpr *CE,
@@ -148,12 +159,22 @@
// 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 +225,7 @@
E = E->IgnoreImplicit();
if (isa<CXXThisExpr>(E)) {
+ // direct use of "this"
B.CM = CM_ByValue;
B.UsageIdentifier = std::string(getSourceTextForExpr(Result, E));
B.CaptureIdentifier = "this";
@@ -217,10 +239,16 @@
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 ((std::distance(ME->child_begin(), ME->child_end()) == 1) &&
+ isa<CXXThisExpr>(*ME->children().begin())) {
+ // reference to data member without 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 @@
B.IsUsed = true;
SmallVector<StringRef, 2> Matches;
- if (MatchPlaceholder.match(B.SourceTokens, &Matches)) {
+ clang::DeclRefExpr const *DRE = dyn_cast<DeclRefExpr>(E);
+ if (MatchPlaceholder.match(B.SourceTokens, &Matches) ||
+ // match against unqualified name
+ (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 @@
// 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 @@
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 @@
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,24 @@
Candidates.push_back(Method);
}
+ // find templated operator(), if any
+ for (const clang::Decl *D : RecordDecl->decls()) {
+ if (D->getKind() != Decl::Kind::FunctionTemplate)
+ continue;
+
+ const clang::FunctionTemplateDecl *FT =
+ clang::dyn_cast<FunctionTemplateDecl>(D);
+ const clang::FunctionDecl *FD = FT->getTemplatedDecl();
+ OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+ if (OOK != OverloadedOperatorKind::OO_Call)
+ continue;
+
+ if (FD->getNumParams() > NumArgs)
+ continue;
+
+ Candidates.push_back(FD);
+ }
+
return Candidates;
}
@@ -407,7 +461,7 @@
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 +520,14 @@
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)))
+ // something that looks like a call, with arguments
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 @@
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 @@
}
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 @@
if (CM == CM_ByRef)
Stream << "&";
Stream << Identifier;
- if (CM == CM_InitExpression)
+ if (CE == CE_InitExpression)
Stream << " = " << InitExpression;
CaptureSet.insert(Identifier);
@@ -550,9 +609,9 @@
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 @@
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;
}
@@ -631,19 +690,18 @@
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);
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
