[clang-tidy][NFC] Refactor bugprone-use-after-move check
#172219
+66
−57
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: mitchell (zeyi2) ChangesThis change is a necessary step for a subsequent PR that will enhance the Part of #172018 Full diff: https://github.com/llvm/llvm-project/pull/172219.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b2e08fe688a1b..295453b093ee6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -31,6 +31,76 @@ using matchers::hasUnevaluatedContext;
namespace {
+static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
+ return anyOf(hasAnyName("::std::move", "::std::forward"),
+ matchers::matchesAnyListedName(InvalidationFunctions));
+}
+
+StatementMatcher
+makeReinitMatcher(const ValueDecl *MovedVariable,
+ llvm::ArrayRef<StringRef> InvalidationFunctions) {
+ const auto DeclRefMatcher =
+ declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref");
+
+ static const auto StandardContainerTypeMatcher =
+ hasType(hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(cxxRecordDecl(hasAnyName(
+ "::std::basic_string", "::std::vector", "::std::deque",
+ "::std::forward_list", "::std::list", "::std::set", "::std::map",
+ "::std::multiset", "::std::multimap", "::std::unordered_set",
+ "::std::unordered_map", "::std::unordered_multiset",
+ "::std::unordered_multimap"))))));
+
+ static const auto StandardResettableOwnerTypeMatcher = hasType(
+ hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
+ hasAnyName("::std::unique_ptr", "::std::shared_ptr",
+ "::std::weak_ptr", "::std::optional", "::std::any"))))));
+
+ // Matches different types of reinitialization.
+ return stmt(
+ anyOf(
+ // Assignment. In addition to the overloaded assignment
+ // operator, test for built-in assignment as well, since
+ // template functions may be instantiated to use std::move() on
+ // built-in types.
+ binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)),
+ // Declaration. We treat this as a type of reinitialization
+ // too, so we don't need to treat it separately.
+ declStmt(hasDescendant(equalsNode(MovedVariable))),
+ // clear() and assign() on standard containers.
+ cxxMemberCallExpr(
+ on(expr(DeclRefMatcher, StandardContainerTypeMatcher)),
+ // To keep the matcher simple, we check for assign() calls
+ // on all standard containers, even though only vector,
+ // deque, forward_list and list have assign(). If assign()
+ // is called on any of the other containers, this will be
+ // flagged by a compile error anyway.
+ callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
+ // reset() on standard smart pointers.
+ cxxMemberCallExpr(on(expr(DeclRefMatcher,
+ StandardResettableOwnerTypeMatcher)),
+ callee(cxxMethodDecl(hasName("reset")))),
+ // Methods that have the [[clang::reinitializes]] attribute.
+ cxxMemberCallExpr(on(DeclRefMatcher),
+ callee(cxxMethodDecl(
+ hasAttr(clang::attr::Reinitializes)))),
+ // Passing variable to a function as a non-const pointer.
+ callExpr(forEachArgumentWithParam(
+ unaryOperator(hasOperatorName("&"),
+ hasUnaryOperand(DeclRefMatcher)),
+ unless(
+ parmVarDecl(hasType(pointsTo(isConstQualified())))))),
+ // Passing variable to a function as a non-const lvalue
+ // reference (unless that function is std::move()).
+ callExpr(forEachArgumentWithParam(
+ traverse(TK_AsIs, DeclRefMatcher),
+ unless(parmVarDecl(hasType(
+ references(qualType(isConstQualified())))))),
+ unless(callee(functionDecl(
+ getNameMatcher(InvalidationFunctions)))))))
+ .bind("reinit");
+}
+
/// Contains information about a use-after-move.
struct UseAfterMove {
// The DeclRefExpr that constituted the use of the object.
@@ -81,11 +151,6 @@ class UseAfterMoveFinder {
} // namespace
-static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
- return anyOf(hasAnyName("::std::move", "::std::forward"),
- matchers::matchesAnyListedName(InvalidationFunctions));
-}
-
// Matches nodes that are
// - Part of a decltype argument or class template argument (we check this by
// seeing if they are children of a TypeLoc), or
@@ -313,63 +378,8 @@ void UseAfterMoveFinder::getReinits(
const CFGBlock *Block, const ValueDecl *MovedVariable,
llvm::SmallPtrSetImpl<const Stmt *> *Stmts,
llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
- auto DeclRefMatcher =
- declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref");
-
- auto StandardContainerTypeMatcher = hasType(hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(cxxRecordDecl(hasAnyName(
- "::std::basic_string", "::std::vector", "::std::deque",
- "::std::forward_list", "::std::list", "::std::set", "::std::map",
- "::std::multiset", "::std::multimap", "::std::unordered_set",
- "::std::unordered_map", "::std::unordered_multiset",
- "::std::unordered_multimap"))))));
-
- auto StandardResettableOwnerTypeMatcher = hasType(
- hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
- hasAnyName("::std::unique_ptr", "::std::shared_ptr",
- "::std::weak_ptr", "::std::optional", "::std::any"))))));
-
- // Matches different types of reinitialization.
- auto ReinitMatcher =
- stmt(anyOf(
- // Assignment. In addition to the overloaded assignment operator,
- // test for built-in assignment as well, since template functions
- // may be instantiated to use std::move() on built-in types.
- binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)),
- // Declaration. We treat this as a type of reinitialization too,
- // so we don't need to treat it separately.
- declStmt(hasDescendant(equalsNode(MovedVariable))),
- // clear() and assign() on standard containers.
- cxxMemberCallExpr(
- on(expr(DeclRefMatcher, StandardContainerTypeMatcher)),
- // To keep the matcher simple, we check for assign() calls
- // on all standard containers, even though only vector,
- // deque, forward_list and list have assign(). If assign()
- // is called on any of the other containers, this will be
- // flagged by a compile error anyway.
- callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
- // reset() on standard smart pointers.
- cxxMemberCallExpr(
- on(expr(DeclRefMatcher, StandardResettableOwnerTypeMatcher)),
- callee(cxxMethodDecl(hasName("reset")))),
- // Methods that have the [[clang::reinitializes]] attribute.
- cxxMemberCallExpr(
- on(DeclRefMatcher),
- callee(cxxMethodDecl(hasAttr(clang::attr::Reinitializes)))),
- // Passing variable to a function as a non-const pointer.
- callExpr(forEachArgumentWithParam(
- unaryOperator(hasOperatorName("&"),
- hasUnaryOperand(DeclRefMatcher)),
- unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))),
- // Passing variable to a function as a non-const lvalue reference
- // (unless that function is std::move()).
- callExpr(forEachArgumentWithParam(
- traverse(TK_AsIs, DeclRefMatcher),
- unless(parmVarDecl(hasType(
- references(qualType(isConstQualified())))))),
- unless(callee(functionDecl(
- getNameMatcher(InvalidationFunctions)))))))
- .bind("reinit");
+ const auto ReinitMatcher =
+ makeReinitMatcher(MovedVariable, InvalidationFunctions);
Stmts->clear();
DeclRefs->clear();
|
Member
|
@llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) ChangesThis change is a necessary step for a subsequent PR that will enhance the Part of #172018 Full diff: https://github.com/llvm/llvm-project/pull/172219.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b2e08fe688a1b..295453b093ee6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -31,6 +31,76 @@ using matchers::hasUnevaluatedContext;
namespace {
+static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
+ return anyOf(hasAnyName("::std::move", "::std::forward"),
+ matchers::matchesAnyListedName(InvalidationFunctions));
+}
+
+StatementMatcher
+makeReinitMatcher(const ValueDecl *MovedVariable,
+ llvm::ArrayRef<StringRef> InvalidationFunctions) {
+ const auto DeclRefMatcher =
+ declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref");
+
+ static const auto StandardContainerTypeMatcher =
+ hasType(hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(cxxRecordDecl(hasAnyName(
+ "::std::basic_string", "::std::vector", "::std::deque",
+ "::std::forward_list", "::std::list", "::std::set", "::std::map",
+ "::std::multiset", "::std::multimap", "::std::unordered_set",
+ "::std::unordered_map", "::std::unordered_multiset",
+ "::std::unordered_multimap"))))));
+
+ static const auto StandardResettableOwnerTypeMatcher = hasType(
+ hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
+ hasAnyName("::std::unique_ptr", "::std::shared_ptr",
+ "::std::weak_ptr", "::std::optional", "::std::any"))))));
+
+ // Matches different types of reinitialization.
+ return stmt(
+ anyOf(
+ // Assignment. In addition to the overloaded assignment
+ // operator, test for built-in assignment as well, since
+ // template functions may be instantiated to use std::move() on
+ // built-in types.
+ binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)),
+ // Declaration. We treat this as a type of reinitialization
+ // too, so we don't need to treat it separately.
+ declStmt(hasDescendant(equalsNode(MovedVariable))),
+ // clear() and assign() on standard containers.
+ cxxMemberCallExpr(
+ on(expr(DeclRefMatcher, StandardContainerTypeMatcher)),
+ // To keep the matcher simple, we check for assign() calls
+ // on all standard containers, even though only vector,
+ // deque, forward_list and list have assign(). If assign()
+ // is called on any of the other containers, this will be
+ // flagged by a compile error anyway.
+ callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
+ // reset() on standard smart pointers.
+ cxxMemberCallExpr(on(expr(DeclRefMatcher,
+ StandardResettableOwnerTypeMatcher)),
+ callee(cxxMethodDecl(hasName("reset")))),
+ // Methods that have the [[clang::reinitializes]] attribute.
+ cxxMemberCallExpr(on(DeclRefMatcher),
+ callee(cxxMethodDecl(
+ hasAttr(clang::attr::Reinitializes)))),
+ // Passing variable to a function as a non-const pointer.
+ callExpr(forEachArgumentWithParam(
+ unaryOperator(hasOperatorName("&"),
+ hasUnaryOperand(DeclRefMatcher)),
+ unless(
+ parmVarDecl(hasType(pointsTo(isConstQualified())))))),
+ // Passing variable to a function as a non-const lvalue
+ // reference (unless that function is std::move()).
+ callExpr(forEachArgumentWithParam(
+ traverse(TK_AsIs, DeclRefMatcher),
+ unless(parmVarDecl(hasType(
+ references(qualType(isConstQualified())))))),
+ unless(callee(functionDecl(
+ getNameMatcher(InvalidationFunctions)))))))
+ .bind("reinit");
+}
+
/// Contains information about a use-after-move.
struct UseAfterMove {
// The DeclRefExpr that constituted the use of the object.
@@ -81,11 +151,6 @@ class UseAfterMoveFinder {
} // namespace
-static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
- return anyOf(hasAnyName("::std::move", "::std::forward"),
- matchers::matchesAnyListedName(InvalidationFunctions));
-}
-
// Matches nodes that are
// - Part of a decltype argument or class template argument (we check this by
// seeing if they are children of a TypeLoc), or
@@ -313,63 +378,8 @@ void UseAfterMoveFinder::getReinits(
const CFGBlock *Block, const ValueDecl *MovedVariable,
llvm::SmallPtrSetImpl<const Stmt *> *Stmts,
llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
- auto DeclRefMatcher =
- declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref");
-
- auto StandardContainerTypeMatcher = hasType(hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(cxxRecordDecl(hasAnyName(
- "::std::basic_string", "::std::vector", "::std::deque",
- "::std::forward_list", "::std::list", "::std::set", "::std::map",
- "::std::multiset", "::std::multimap", "::std::unordered_set",
- "::std::unordered_map", "::std::unordered_multiset",
- "::std::unordered_multimap"))))));
-
- auto StandardResettableOwnerTypeMatcher = hasType(
- hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
- hasAnyName("::std::unique_ptr", "::std::shared_ptr",
- "::std::weak_ptr", "::std::optional", "::std::any"))))));
-
- // Matches different types of reinitialization.
- auto ReinitMatcher =
- stmt(anyOf(
- // Assignment. In addition to the overloaded assignment operator,
- // test for built-in assignment as well, since template functions
- // may be instantiated to use std::move() on built-in types.
- binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)),
- // Declaration. We treat this as a type of reinitialization too,
- // so we don't need to treat it separately.
- declStmt(hasDescendant(equalsNode(MovedVariable))),
- // clear() and assign() on standard containers.
- cxxMemberCallExpr(
- on(expr(DeclRefMatcher, StandardContainerTypeMatcher)),
- // To keep the matcher simple, we check for assign() calls
- // on all standard containers, even though only vector,
- // deque, forward_list and list have assign(). If assign()
- // is called on any of the other containers, this will be
- // flagged by a compile error anyway.
- callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
- // reset() on standard smart pointers.
- cxxMemberCallExpr(
- on(expr(DeclRefMatcher, StandardResettableOwnerTypeMatcher)),
- callee(cxxMethodDecl(hasName("reset")))),
- // Methods that have the [[clang::reinitializes]] attribute.
- cxxMemberCallExpr(
- on(DeclRefMatcher),
- callee(cxxMethodDecl(hasAttr(clang::attr::Reinitializes)))),
- // Passing variable to a function as a non-const pointer.
- callExpr(forEachArgumentWithParam(
- unaryOperator(hasOperatorName("&"),
- hasUnaryOperand(DeclRefMatcher)),
- unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))),
- // Passing variable to a function as a non-const lvalue reference
- // (unless that function is std::move()).
- callExpr(forEachArgumentWithParam(
- traverse(TK_AsIs, DeclRefMatcher),
- unless(parmVarDecl(hasType(
- references(qualType(isConstQualified())))))),
- unless(callee(functionDecl(
- getNameMatcher(InvalidationFunctions)))))))
- .bind("reinit");
+ const auto ReinitMatcher =
+ makeReinitMatcher(MovedVariable, InvalidationFunctions);
Stmts->clear();
DeclRefs->clear();
|
|
✅ With the latest revision this PR passed the C/C++ code linter. |
vbvictor
approved these changes
Dec 15, 2025
Contributor
vbvictor
left a comment
vbvictor
left a comment
There was a problem hiding this comment.
Apart from one comment, lgtm
| const auto DeclRefMatcher = | ||
| declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); | ||
|
|
||
| static const auto StandardContainerTypeMatcher = |
Contributor
There was a problem hiding this comment.
Does it matter here to add static? AFAIK we call makeReinitMatcher only once and I'm not sure how matchers will behave if multiple matchers are constructed with same static submathers. I'd remove static for now to be on the safe side.
localspook
reviewed
Dec 16, 2025
| } | ||
|
|
||
| /// Contains information about a use-after-move. | ||
| struct UseAfterMove { |
Contributor
There was a problem hiding this comment.
We shouldn't delete the anonymous namespace entirely, because we still want to keep this struct UseAfterMoveFinder (and class UseAfterMoveFinder a little lower) file-local.
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is a necessary step for a subsequent PR that will enhance the
bugprone-use-after-movecheck to correctly handle cases where variables are re-initialized inside captured lambdas, which currently lead to FPs.Part of #172018