https://github.com/PiotrZSL created
https://github.com/llvm/llvm-project/pull/94869
- Removed custom smart pointers handling (were hiding issues)
- Changed 'move occurred here' note location to always point to 'std::move'
Closes #90174
>From 1179f63d792da2462e1490c1b0a59cf1e6e47349 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <m...@piotrzegar.pl>
Date: Sat, 8 Jun 2024 19:41:51 +0000
Subject: [PATCH 1/3] [clang-tidy] Fix smart pointers handling in
bugprone-use-after-move
- Removed custom smart pointers handling (were hidding issues)
- Changed 'move occurred here' note location to always point to 'std::move'
- Fixed issue when 'std::move' of 'x' on initialization list would be
incorrectly detected as use after move.
Closes #90174
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 39 +-----
.../checkers/bugprone/use-after-move.cpp | 114 ++++++++++++++----
2 files changed, 95 insertions(+), 58 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..593ed11d8ac43 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -215,26 +215,6 @@ void UseAfterMoveFinder::getUsesAndReinits(
});
}
-bool isStandardSmartPointer(const ValueDecl *VD) {
- const Type *TheType = VD->getType().getNonReferenceType().getTypePtrOrNull();
- if (!TheType)
- return false;
-
- const CXXRecordDecl *RecordDecl = TheType->getAsCXXRecordDecl();
- if (!RecordDecl)
- return false;
-
- const IdentifierInfo *ID = RecordDecl->getIdentifier();
- if (!ID)
- return false;
-
- StringRef Name = ID->getName();
- if (Name != "unique_ptr" && Name != "shared_ptr" && Name != "weak_ptr")
- return false;
-
- return RecordDecl->getDeclContext()->isStdNamespace();
-}
-
void UseAfterMoveFinder::getDeclRefs(
const CFGBlock *Block, const Decl *MovedVariable,
llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
@@ -248,13 +228,8 @@ void UseAfterMoveFinder::getDeclRefs(
DeclRefs](const ArrayRef<BoundNodes> Matches) {
for (const auto &Match : Matches) {
const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref");
- const auto *Operator =
Match.getNodeAs<CXXOperatorCallExpr>("operator");
if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) {
- // Ignore uses of a standard smart pointer that don't dereference the
- // pointer.
- if (Operator || !isStandardSmartPointer(DeclRef->getDecl())) {
- DeclRefs->insert(DeclRef);
- }
+ DeclRefs->insert(DeclRef);
}
}
};
@@ -265,11 +240,6 @@ void UseAfterMoveFinder::getDeclRefs(
AddDeclRefs(match(traverse(TK_AsIs, findAll(DeclRefMatcher)),
*S->getStmt(),
*Context));
- AddDeclRefs(match(findAll(cxxOperatorCallExpr(
- hasAnyOverloadedOperatorName("*", "->",
"[]"),
- hasArgument(0, DeclRefMatcher))
- .bind("operator")),
- *S->getStmt(), *Context));
}
}
@@ -411,10 +381,9 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder
*Finder) {
auto TryEmplaceMatcher =
cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
auto CallMoveMatcher =
- callExpr(argumentCountIs(1),
+ callExpr(argumentCountIs(1), hasArgument(0, declRefExpr().bind("arg")),
callee(functionDecl(hasAnyName("::std::move", "::std::forward"))
.bind("move-decl")),
- hasArgument(0, declRefExpr().bind("arg")),
unless(inDecltypeOrTemplateArg()),
unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
anyOf(hasAncestor(compoundStmt(
@@ -464,7 +433,7 @@ void UseAfterMoveCheck::check(const
MatchFinder::MatchResult &Result) {
const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl");
if (!MovingCall || !MovingCall->getExprLoc().isValid())
- MovingCall = CallMove;
+ MovingCall = ContainingCtorInit ? ContainingCtorInit : CallMove;
// Ignore the std::move if the variable that was passed to it isn't a local
// variable.
@@ -496,7 +465,7 @@ void UseAfterMoveCheck::check(const
MatchFinder::MatchResult &Result) {
UseAfterMoveFinder Finder(Result.Context);
UseAfterMove Use;
if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
- emitDiagnostic(MovingCall, Arg, Use, this, Result.Context,
+ emitDiagnostic(CallMove, Arg, Use, this, Result.Context,
determineMoveType(MoveDecl));
}
}
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 7d9f63479a1b4..3d06dc008d353 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -8,6 +8,7 @@ typedef unsigned size_t;
template <typename T>
struct unique_ptr {
unique_ptr();
+ unique_ptr(T* ptr);
T *get() const;
explicit operator bool() const;
void reset(T *ptr);
@@ -123,6 +124,10 @@ forward(typename std::remove_reference<_Tp>::type&& __t)
noexcept {
return static_cast<_Tp&&>(__t);
}
+template <typename T, typename... Args> auto make_unique(Args &&...args) {
+ return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
+}
+
} // namespace std
class A {
@@ -220,10 +225,8 @@ void standardSmartPtr() {
std::unique_ptr<A> ptr;
std::move(ptr);
ptr.get();
- static_cast<bool>(ptr);
- *ptr;
- // CHECK-NOTES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved
- // CHECK-NOTES: [[@LINE-5]]:5: note: move occurred here
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
{
std::unique_ptr<A> ptr;
@@ -243,10 +246,8 @@ void standardSmartPtr() {
std::shared_ptr<A> ptr;
std::move(ptr);
ptr.get();
- static_cast<bool>(ptr);
- *ptr;
- // CHECK-NOTES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved
- // CHECK-NOTES: [[@LINE-5]]:5: note: move occurred here
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
{
std::shared_ptr<A> ptr;
@@ -261,6 +262,8 @@ void standardSmartPtr() {
std::weak_ptr<A> ptr;
std::move(ptr);
ptr.expired();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
// Make sure we recognize std::unique_ptr<> or std::shared_ptr<> if they're
// wrapped in a typedef.
@@ -269,12 +272,16 @@ void standardSmartPtr() {
PtrToA ptr;
std::move(ptr);
ptr.get();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
{
typedef std::shared_ptr<A> PtrToA;
PtrToA ptr;
std::move(ptr);
ptr.get();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
// And we don't get confused if the template argument is a little more
// involved.
@@ -285,6 +292,8 @@ void standardSmartPtr() {
std::unique_ptr<B::AnotherNameForA> ptr;
std::move(ptr);
ptr.get();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
// Make sure we treat references to smart pointers correctly.
{
@@ -292,12 +301,16 @@ void standardSmartPtr() {
std::unique_ptr<A>& ref_to_ptr = ptr;
std::move(ref_to_ptr);
ref_to_ptr.get();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ref_to_ptr' used after it was
moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
{
std::unique_ptr<A> ptr;
std::unique_ptr<A>&& rvalue_ref_to_ptr = std::move(ptr);
std::move(rvalue_ref_to_ptr);
rvalue_ref_to_ptr.get();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'rvalue_ref_to_ptr' used after it
was moved
+ // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
}
// We don't give any special treatment to types that are called "unique_ptr"
// or "shared_ptr" but are not in the "::std" namespace.
@@ -329,7 +342,7 @@ void moveInDeclaration() {
A another_a(std::move(a));
a.foo();
// CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
}
// We see the std::move if it's inside an initializer list. Initializer lists
@@ -1068,10 +1081,10 @@ void sequencingOfMoveAndUse() {
A a;
g(g(a, std::move(a)), g(a, std::move(a)));
// CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:27: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:32: note: move occurred here
// CHECK-NOTES: [[@LINE-3]]:9: note: the use and move are unsequenced
// CHECK-NOTES: [[@LINE-4]]:29: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-5]]:7: note: move occurred here
+ // CHECK-NOTES: [[@LINE-5]]:12: note: move occurred here
// CHECK-NOTES: [[@LINE-6]]:29: note: the use and move are unsequenced
}
// This case is fine because the actual move only happens inside the call to
@@ -1088,7 +1101,7 @@ void sequencingOfMoveAndUse() {
int v[3];
v[a.getInt()] = intFromA(std::move(a));
// CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:21: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:30: note: move occurred here
// CHECK-NOTES: [[@LINE-3]]:7: note: the use and move are unsequenced
}
{
@@ -1096,7 +1109,7 @@ void sequencingOfMoveAndUse() {
int v[3];
v[intFromA(std::move(a))] = intFromInt(a.i);
// CHECK-NOTES: [[@LINE-1]]:44: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:7: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:16: note: move occurred here
// CHECK-NOTES: [[@LINE-3]]:44: note: the use and move are unsequenced
}
}
@@ -1168,7 +1181,7 @@ void commaOperatorSequences() {
(a = A()), A(std::move(a));
a.foo();
// CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-3]]:16: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:18: note: move occurred here
}
}
@@ -1210,7 +1223,7 @@ void initializerListSequences() {
A a;
S2 s2{.a = std::move(a), .i = a.getInt()};
// CHECK-NOTES: [[@LINE-1]]:35: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:16: note: move occurred here
}
{
// Check the case where the constructed type has a constructor and the
@@ -1264,7 +1277,7 @@ void logicalOperatorsSequence() {
A a;
if (A(std::move(a)).getInt() > 0 && a.getInt() > 0) {
// CHECK-NOTES: [[@LINE-1]]:41: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
A().foo();
}
}
@@ -1278,7 +1291,7 @@ void logicalOperatorsSequence() {
A a;
if (A(std::move(a)).getInt() > 0 || a.getInt() > 0) {
// CHECK-NOTES: [[@LINE-1]]:41: warning: 'a' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
A().foo();
}
}
@@ -1324,7 +1337,7 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() {
for (int i = 0; i < 10; ++i) {
if (A a1; A(std::move(a1)).getInt() > a1.getInt()) {}
// CHECK-NOTES: [[@LINE-1]]:43: warning: 'a1' used after it was moved
- // CHECK-NOTES: [[@LINE-2]]:15: note: move occurred here
+ // CHECK-NOTES: [[@LINE-2]]:17: note: move occurred here
// CHECK-NOTES: [[@LINE-3]]:43: note: the use and move are unsequenced
}
for (int i = 0; i < 10; ++i) {
@@ -1419,7 +1432,7 @@ class CtorInit {
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
+ // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
{}
private:
@@ -1435,7 +1448,7 @@ class CtorInitLambda {
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
+ // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
c{[] {
std::string str{};
std::move(str);
@@ -1445,7 +1458,7 @@ class CtorInitLambda {
}()} {
std::move(val);
// CHECK-NOTES: [[@LINE-1]]:15: warning: 'val' used after it was moved
- // CHECK-NOTES: [[@LINE-13]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-13]]:11: note: move occurred here
std::string val2{};
std::move(val2);
val2.empty();
@@ -1468,7 +1481,7 @@ class CtorInitOrder {
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-1]]:11: note: move occurred here
// CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop
iteration than the move
private:
@@ -1531,7 +1544,7 @@ class PR38187 {
PR38187(std::string val) : val_(std::move(val)) {
val.empty();
// CHECK-NOTES: [[@LINE-1]]:5: warning: 'val' used after it was moved
- // CHECK-NOTES: [[@LINE-3]]:30: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:35: note: move occurred here
}
private:
@@ -1562,3 +1575,58 @@ void create() {
}
} // namespace issue82023
+
+namespace PR90174 {
+
+struct A {};
+
+struct SinkA {
+ SinkA(std::unique_ptr<A>);
+};
+
+class ClassB {
+ ClassB(std::unique_ptr<A> aaa) : aa(std::move(aaa)) {
+ a = std::make_unique<SinkA>(std::move(aaa));
+ // CHECK-NOTES: [[@LINE-1]]:43: warning: 'aaa' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:39: note: move occurred here
+ }
+ std::unique_ptr<A> aa;
+ std::unique_ptr<SinkA> a;
+};
+
+void s(const std::unique_ptr<A> &);
+
+template <typename T, typename... Args> auto my_make_unique(Args &&...args) {
+ return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
+}
+
+void natively(std::unique_ptr<A> x) {
+ std::unique_ptr<A> tmp = std::move(x);
+ std::unique_ptr<SinkA> y2{new SinkA(std::move(x))};
+ // CHECK-NOTES: [[@LINE-1]]:49: warning: 'x' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:28: note: move occurred here
+}
+
+void viaStdMakeUnique(std::unique_ptr<A> x) {
+ std::unique_ptr<A> tmp = std::move(x);
+ std::unique_ptr<SinkA> y2 =
+ std::make_unique<SinkA>(std::move(x));
+ // CHECK-NOTES: [[@LINE-1]]:41: warning: 'x' used after it was moved
+ // CHECK-NOTES: [[@LINE-4]]:28: note: move occurred here
+}
+
+void viaMyMakeUnique(std::unique_ptr<A> x) {
+ std::unique_ptr<A> tmp = std::move(x);
+ std::unique_ptr<SinkA> y2 = my_make_unique<SinkA>(std::move(x));
+ // CHECK-NOTES: [[@LINE-1]]:63: warning: 'x' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:28: note: move occurred here
+}
+
+void viaMyMakeUnique2(std::unique_ptr<A> x) {
+ std::unique_ptr<A> tmp = std::move(x);
+ s(x);
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'x' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:28: note: move occurred here
+}
+
+}
>From 30e5074e9727661dff24ee0b190b1e89a3f622de Mon Sep 17 00:00:00 2001
From: Piotr Zegar <m...@piotrzegar.pl>
Date: Sat, 8 Jun 2024 19:49:04 +0000
Subject: [PATCH 2/3] Add release notes & documentation
---
clang-tools-extra/docs/ReleaseNotes.rst | 3 ++-
.../docs/clang-tidy/checks/bugprone/use-after-move.rst | 7 -------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 277a6e75da2ac..f9b84a6df532e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -254,7 +254,8 @@ Changes in existing checks
- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` check to also handle
- calls to ``std::forward``.
+ calls to ``std::forward``. Smart pointers are now handled like any other
+ objects allowing to detect more cases.
- Improved :doc:`cppcoreguidelines-missing-std-forward
<clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
index 08bb5374bab1f..faf9e4dddc12c 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
@@ -195,13 +195,6 @@ Use
Any occurrence of the moved variable that is not a reinitialization (see below)
is considered to be a use.
-An exception to this are objects of type ``std::unique_ptr``,
-``std::shared_ptr`` and ``std::weak_ptr``, which have defined move behavior
-(objects of these classes are guaranteed to be empty after they have been moved
-from). Therefore, an object of these classes will only be considered to be used
-if it is dereferenced, i.e. if ``operator*``, ``operator->`` or ``operator[]``
-(in the case of ``std::unique_ptr<T []>``) is called on it.
-
If multiple uses occur after a move, only the first of these is flagged.
Reinitialization
>From dfad1e2ff4495d50d5da8f5604550dc367579f3c Mon Sep 17 00:00:00 2001
From: Piotr Zegar <m...@piotrzegar.pl>
Date: Sat, 8 Jun 2024 19:58:13 +0000
Subject: [PATCH 3/3] Remove not needed change
---
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index 593ed11d8ac43..4f1ea32da20f4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -433,7 +433,7 @@ void UseAfterMoveCheck::check(const
MatchFinder::MatchResult &Result) {
const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl");
if (!MovingCall || !MovingCall->getExprLoc().isValid())
- MovingCall = ContainingCtorInit ? ContainingCtorInit : CallMove;
+ MovingCall = CallMove;
// Ignore the std::move if the variable that was passed to it isn't a local
// variable.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
