MarcoFalke updated this revision to Diff 506586.
MarcoFalke marked an inline comment as done.
MarcoFalke added a comment.
Took some review suggestions (Thanks!)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146288/new/
https://reviews.llvm.org/D146288
Files:
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -369,6 +369,18 @@
};
a.foo();
}
+ // Don't warn if 'a' is a copy inside a synchronous lambda
+ {
+ A a;
+ A copied{[a] mutable { return std::move(a); }()};
+ a.foo();
+ }
+ // False negative (should warn if 'a' is a ref inside a synchronous lambda)
+ {
+ A a;
+ A moved{[&a] mutable { return std::move(a); }()};
+ a.foo();
+ }
// Warn if the use consists of a capture that happens after a move.
{
A a;
@@ -1353,6 +1365,120 @@
}
} // namespace UnevalContext
+class CtorInit {
+public:
+ CtorInit(std::string val)
+ : a{val.empty()}, // fine
+ s{std::move(val)},
+ b{val.empty()}
+ // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here
+ {}
+
+private:
+ bool a;
+ std::string s;
+ bool b;
+};
+
+class CtorInitLambda {
+public:
+ CtorInitLambda(std::string val)
+ : a{val.empty()}, // fine
+ s{std::move(val)},
+ b{[&] { return val.empty(); }()},
+ // CHECK-NOTES: [[@LINE-1]]:12: warning: 'val' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here
+ c{[] {
+ std::string str{};
+ std::move(str);
+ return str.empty();
+ // CHECK-NOTES: [[@LINE-1]]:18: warning: 'str' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
+ }()} {
+ std::move(val);
+ // CHECK-NOTES: [[@LINE-1]]:15: warning: 'val' used after it was moved
+ // CHECK-NOTES: [[@LINE-13]]:9: note: move occurred here
+ std::string val2{};
+ std::move(val2);
+ val2.empty();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val2' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+ }
+
+private:
+ bool a;
+ std::string s;
+ bool b;
+ bool c;
+ bool d{};
+};
+
+class CtorInitOrder {
+public:
+ CtorInitOrder(std::string val)
+ : a{val.empty()}, // fine
+ b{val.empty()},
+ // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
+ s{std::move(val)} {} // wrong order
+ // CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move
+
+private:
+ bool a;
+ std::string s;
+ bool b;
+};
+
+struct Obj {};
+struct CtorD {
+ CtorD(Obj b);
+};
+
+struct CtorC {
+ CtorC(Obj b);
+};
+
+struct CtorB {
+ CtorB(Obj &b);
+};
+
+struct CtorA : CtorB, CtorC, CtorD {
+ CtorA(Obj b) : CtorB{b}, CtorC{std::move(b)}, CtorD{b} {}
+ // CHECK-NOTES: [[@LINE-1]]:55: warning: 'b' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:34: note: move occurred here
+};
+
+struct Base {
+ Base(Obj b) : bb{std::move(b)} {}
+ template <typename Call> Base(Call &&c) : bb{c()} {};
+
+ Obj bb;
+};
+
+// struct Derived : Base, CtorC {
+// Derived(Obj b) : Base{[&] mutable { return std::move(b); }()}, CtorC{b} {}
+// };
+
+struct Derived2 : Base, CtorC {
+ Derived2(Obj b)
+ // TODO? is this a false positive, considering that we don't know whether Base(Call &&c) will actually call 'c'?
+ : Base{[&] mutable { return std::move(b); }},
+ // CHECK-NOTES: [[@LINE-1]]:15: warning: 'b' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:35: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:15: note: the use and move are unsequenced, i.e. there is no guarantee about the order in which they are evaluated
+ CtorC{b} {}
+ // CHECK-NOTES: [[@LINE-1]]:15: warning: 'b' used after it was moved
+ // CHECK-NOTES: [[@LINE-6]]:35: note: move occurred here
+};
+
+struct Derived3 : Base, CtorC {
+ Derived3(Obj b)
+ : Base{[c = std::move(b)] mutable { return std::move(c); }}, CtorC{b} {}
+ // CHECK-NOTES: [[@LINE-1]]:74: warning: 'b' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:19: note: move occurred here
+};
+
class PR38187 {
public:
PR38187(std::string val) : val_(std::move(val)) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -162,6 +162,10 @@
<clang-tidy/checks/bugprone/suspicious-include>` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
+ initializers.
+
- Deprecated check-local options `HeaderFileExtensions`
in :doc:`google-build-namespaces
<clang-tidy/checks/google/build-namespaces>` check.
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -58,11 +58,11 @@
public:
UseAfterMoveFinder(ASTContext *TheContext);
- // Within the given function body, finds the first use of 'MovedVariable' that
+ // Within the given code block, finds the first use of 'MovedVariable' that
// occurs after 'MovingCall' (the expression that performs the move). If a
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
- bool find(Stmt *FunctionBody, const Expr *MovingCall,
+ bool find(Stmt *CodeBlock, const Expr *MovingCall,
const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
@@ -104,7 +104,7 @@
UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
: Context(TheContext) {}
-bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
+bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
const ValueDecl *MovedVariable,
UseAfterMove *TheUseAfterMove) {
// Generate the CFG manually instead of through an AnalysisDeclContext because
@@ -118,12 +118,11 @@
Options.AddImplicitDtors = true;
Options.AddTemporaryDtors = true;
std::unique_ptr<CFG> TheCFG =
- CFG::buildCFG(nullptr, FunctionBody, Context, Options);
+ CFG::buildCFG(nullptr, CodeBlock, Context, Options);
if (!TheCFG)
return false;
- Sequence =
- std::make_unique<ExprSequence>(TheCFG.get(), FunctionBody, Context);
+ Sequence = std::make_unique<ExprSequence>(TheCFG.get(), CodeBlock, Context);
BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
Visited.clear();
@@ -398,20 +397,29 @@
}
void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
- auto CallMoveMatcher =
- callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
- hasArgument(0, declRefExpr().bind("arg")),
- anyOf(hasAncestor(compoundStmt(
- hasParent(lambdaExpr().bind("containing-lambda")))),
- hasAncestor(functionDecl().bind("containing-func"))),
- unless(inDecltypeOrTemplateArg()),
- // try_emplace is a common maybe-moving function that returns a
- // bool to tell callers whether it moved. Ignore std::move inside
- // try_emplace to avoid false positives as we don't track uses of
- // the bool.
- unless(hasParent(cxxMemberCallExpr(
- callee(cxxMethodDecl(hasName("try_emplace")))))))
- .bind("call-move");
+ // try_emplace is a common maybe-moving function that returns a
+ // bool to tell callers whether it moved. Ignore std::move inside
+ // try_emplace to avoid false positives as we don't track uses of
+ // the bool.
+ auto TryEmplaceMatcher =
+ cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
+ auto CallMoveMatcher = callExpr(
+ argumentCountIs(
+ 1), // Match count first, as it is faster than matching name
+ callee(functionDecl(hasName("::std::move"))),
+ hasArgument(0, declRefExpr().bind("arg")),
+ unless(inDecltypeOrTemplateArg()), unless(hasParent(TryEmplaceMatcher)),
+ expr().bind("call-move"),
+ anyOf(hasAncestor(compoundStmt(
+ hasParent(lambdaExpr().bind("containing-lambda")))),
+ hasAncestor(
+ expr(
+ // For some reason cxxCtorInitializer didn't
+ // match, so work around by matching the topmost
+ // Expr inside the CXXCtorInitializer.
+ hasParent(cxxConstructorDecl().bind("containing-ctor")))
+ .bind("containing-ctor-init")),
+ hasAncestor(functionDecl().bind("containing-func"))));
Finder->addMatcher(
traverse(
@@ -434,6 +442,10 @@
}
void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *ContainingCtor =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("containing-ctor");
+ const auto *ContainingCtorInit =
+ Result.Nodes.getNodeAs<Expr>("containing-ctor-init");
const auto *ContainingLambda =
Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
const auto *ContainingFunc =
@@ -445,23 +457,49 @@
if (!MovingCall || !MovingCall->getExprLoc().isValid())
MovingCall = CallMove;
- Stmt *FunctionBody = nullptr;
- if (ContainingLambda)
- FunctionBody = ContainingLambda->getBody();
- else if (ContainingFunc)
- FunctionBody = ContainingFunc->getBody();
- else
- return;
-
// Ignore the std::move if the variable that was passed to it isn't a local
// variable.
if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod())
return;
- UseAfterMoveFinder Finder(Result.Context);
- UseAfterMove Use;
- if (Finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use))
- emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+ // Collect all code blocks that could use the arg after move.
+ llvm::SmallVector<Stmt *> CodeBlocks{};
+ if (ContainingCtor) {
+ CodeBlocks.push_back(ContainingCtor->getBody());
+ if (ContainingCtorInit) {
+ // Find the constructor initializer's member field.
+ FieldDecl *ContainingCtorField{nullptr};
+ for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+ if (Init->getInit()->IgnoreImplicit() ==
+ ContainingCtorInit->IgnoreImplicit()) {
+ ContainingCtorField = Init->getMember();
+ break;
+ }
+ }
+ if (ContainingCtorField || true) {
+ // Collect the constructor initializer expressions.
+ bool BeforeMove{true};
+ for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+ if (BeforeMove && Init->getInit()->IgnoreImplicit() ==
+ ContainingCtorInit->IgnoreImplicit())
+ BeforeMove = false;
+ if (!BeforeMove && Init->getInit())
+ CodeBlocks.push_back(Init->getInit());
+ }
+ }
+ }
+ } else if (ContainingLambda)
+ CodeBlocks.push_back(ContainingLambda->getBody());
+ else if (ContainingFunc) {
+ CodeBlocks.push_back(ContainingFunc->getBody());
+ }
+
+ for (Stmt *CodeBlock : CodeBlocks) {
+ UseAfterMoveFinder Finder(Result.Context);
+ UseAfterMove Use;
+ if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+ emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+ }
}
} // namespace clang::tidy::bugprone
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
