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 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 @@ 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 @@ } // 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(E)) return ignoreTemporariesAndPointers(T->getSubExpr()); @@ -148,12 +160,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 +226,7 @@ E = E->IgnoreImplicit(); if (isa(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 @@ if (!ME->isLValue() || !isa(ME->getMemberDecl())) return false; - B.CM = CM_ByValue; - B.UsageIdentifier = std::string(getSourceTextForExpr(Result, E)); - B.CaptureIdentifier = "this"; - return true; + if (isa(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 @@ -251,7 +279,10 @@ B.IsUsed = true; SmallVector Matches; - if (MatchPlaceholder.match(B.SourceTokens, &Matches)) { + const auto *DRE = dyn_cast(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 @@ // 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"; + 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 +static std::vector findCandidateCallOperators(const CXXRecordDecl *RecordDecl, size_t NumArgs) { - std::vector Candidates; + std::vector Candidates; for (const clang::CXXMethodDecl *Method : RecordDecl->methods()) { OverloadedOperatorKind OOK = Method->getOverloadedOperator(); @@ -373,6 +409,23 @@ Candidates.push_back(Method); } + // Find templated operator(), if any. + for (const clang::Decl *D : RecordDecl->decls()) { + const auto *FTD = dyn_cast(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 @@ const FunctionDecl *getCallOperator(const CXXRecordDecl *Callable, size_t NumArgs) { - std::vector Candidates = + std::vector Candidates = findCandidateCallOperators(Callable, NumArgs); if (Candidates.size() != 1) return nullptr; @@ -466,11 +519,15 @@ const auto *NoTemporaries = ignoreTemporariesAndPointers(CallableExpr); - if (isa(NoTemporaries)) + const auto *CE = dyn_cast(NoTemporaries); + const auto *FC = dyn_cast(NoTemporaries); + if ((isa(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(NoTemporaries) || - isa(NoTemporaries)) + if (isa(NoTemporaries) || CE) return CMK_Function; if (const auto *DRE = dyn_cast(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); } } 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(PH1), 2); }; } 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 bind_rt bind(Fp &&, Arguments &&...); -} +} // namespace impl template T ref(T &t); -} +} // namespace std namespace boost { template @@ -58,12 +58,33 @@ void UseF(F); +struct G { + G() : _member(0) {} + G(int m) : _member(m) {} + + template + void operator()(T) const {} + + int _member; +}; + +template +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 +void testMemberFnOfClassTemplate(T) { + auto HHH = std::bind(H(), 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()(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(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(PH2), std::forward(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(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(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(PH1)); }; } struct E {