llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Piotr Zegar (PiotrZSL) <details> <summary>Changes</summary> - Removed custom smart pointers handling (were hiding issues) - Changed 'move occurred here' note location to always point to 'std::move' Closes #<!-- -->90174 --- Full diff: https://github.com/llvm/llvm-project/pull/94869.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+3-34) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1) - (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst (-7) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp (+91-23) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index b91ad0f182295..4f1ea32da20f4 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( @@ -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/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 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 +} + +} `````````` </details> https://github.com/llvm/llvm-project/pull/94869 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
