Skip to content

Commit

Permalink
Repair various issues with modernize-avoid-bind
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jefftrull @AaronBallman
jefftrull authored and AaronBallman committed Jun 25, 2020
1 parent c95ffad commit 95a3550
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 37 deletions.
120 changes: 89 additions & 31 deletions clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
View file
Expand Up @@ -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
Expand All @@ -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;

Expand All @@ -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;
Expand All @@ -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());
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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";
Expand All @@ -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>
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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 = ", ";
Expand All @@ -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();
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -550,17 +609,17 @@ 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)
continue;

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;
}
Expand Down Expand Up @@ -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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...g-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
View file
Expand Up @@ -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); };
}

0 comments on commit 95a3550

Please sign in to comment.