Author: mboehme Date: Wed Nov 2 12:34:47 2016 New Revision: 285842 URL: http://llvm.org/viewvc/llvm-project?rev=285842&view=rev Log: [clang-tidy] Extend misc-use-after-move to support unique_ptr and shared_ptr.
Summary: As a unique_ptr or shared_ptr that has been moved from is guaranteed to be null, we only warn if the pointer is dereferenced. Reviewers: hokein, alexfh, aaron.ballman Subscribers: Prazek, cfe-commits Differential Revision: https://reviews.llvm.org/D26041 Modified: clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp Modified: clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp?rev=285842&r1=285841&r2=285842&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp Wed Nov 2 12:34:47 2016 @@ -463,6 +463,26 @@ void UseAfterMoveFinder::getUsesAndReini }); } +bool isStandardSmartPointer(const ValueDecl *VD) { + const Type *TheType = VD->getType().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) { @@ -472,17 +492,33 @@ void UseAfterMoveFinder::getDeclRefs( if (!S) continue; - SmallVector<BoundNodes, 1> Matches = - match(findAll(declRefExpr(hasDeclaration(equalsNode(MovedVariable)), - unless(inDecltypeOrTemplateArg())) - .bind("declref")), - *S->getStmt(), *Context); + auto addDeclRefs = [this, Block, + 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); + } + } + } + }; - for (const auto &Match : Matches) { - const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref"); - if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) - DeclRefs->insert(DeclRef); - } + auto DeclRefMatcher = declRefExpr(hasDeclaration(equalsNode(MovedVariable)), + unless(inDecltypeOrTemplateArg())) + .bind("declref"); + + addDeclRefs(match(findAll(DeclRefMatcher), *S->getStmt(), *Context)); + addDeclRefs(match( + findAll(cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("*"), + hasOverloadedOperatorName("->"), + hasOverloadedOperatorName("[]")), + hasArgument(0, DeclRefMatcher)) + .bind("operator")), + *S->getStmt(), *Context)); } } @@ -500,6 +536,9 @@ void UseAfterMoveFinder::getReinits( "::std::unordered_set", "::std::unordered_map", "::std::unordered_multiset", "::std::unordered_multimap"))); + auto StandardSmartPointerTypeMatcher = hasType(cxxRecordDecl( + hasAnyName("::std::unique_ptr", "::std::shared_ptr", "::std::weak_ptr"))); + // Matches different types of reinitialization. auto ReinitMatcher = stmt(anyOf( @@ -521,6 +560,10 @@ void UseAfterMoveFinder::getReinits( // 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(allOf(DeclRefMatcher, StandardSmartPointerTypeMatcher)), + callee(cxxMethodDecl(hasName("reset")))), // Passing variable to a function as a non-const pointer. callExpr(forEachArgumentWithParam( unaryOperator(hasOperatorName("&"), @@ -587,15 +630,10 @@ void UseAfterMoveCheck::registerMatchers if (!getLangOpts().CPlusPlus11) return; - auto StandardSmartPointerTypeMatcher = hasType( - cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"))); - auto CallMoveMatcher = callExpr( callee(functionDecl(hasName("::std::move"))), argumentCountIs(1), - hasArgument( - 0, - declRefExpr(unless(StandardSmartPointerTypeMatcher)).bind("arg")), + hasArgument(0, declRefExpr().bind("arg")), anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")), hasAncestor(functionDecl().bind("containing-func"))), unless(inDecltypeOrTemplateArg())) Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst?rev=285842&r1=285841&r2=285842&view=diff ============================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst (original) +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst Wed Nov 2 12:34:47 2016 @@ -75,10 +75,6 @@ move: std::cout << str; } -No warnings are emitted for objects of type ``std::unique_ptr`` and -``std::shared_ptr``, as they have defined move behavior. (Objects of these -classes are guaranteed to be empty after they have been moved from.) - Subsections below explain more precisely what exactly the check considers to be a move, use, and reinitialization. @@ -153,6 +149,13 @@ 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 @@ -172,6 +175,9 @@ The check considers a variable to be rei ``unordered_set``, ``unordered_map``, ``unordered_multiset``, ``unordered_multimap``. + - ``reset()`` is called on the variable and the variable is of type + ``std::unique_ptr``, ``std::shared_ptr`` or ``std::weak_ptr``. + If the variable in question is a struct and an individual member variable of that struct is written to, the check does not consider this to be a reinitialization -- even if, eventually, all member variables of the struct are Modified: clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp?rev=285842&r1=285841&r2=285842&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp Wed Nov 2 12:34:47 2016 @@ -9,12 +9,27 @@ template <typename T> struct unique_ptr { unique_ptr(); T *get() const; + explicit operator bool() const; + void reset(T *ptr); + T &operator*() const; + T *operator->() const; + T& operator[](size_t i) const; }; template <typename T> struct shared_ptr { shared_ptr(); T *get() const; + explicit operator bool() const; + void reset(T *ptr); + T &operator*() const; + T *operator->() const; +}; + +template <typename T> +struct weak_ptr { + weak_ptr(); + bool expired() const; }; #define DECLARE_STANDARD_CONTAINER(name) \ @@ -146,20 +161,58 @@ void parameters(A a) { // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here } -void uniquePtrAndSharedPtr() { - // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged. +void standardSmartPtr() { + // std::unique_ptr<>, std::shared_ptr<> and std::weak_ptr<> are guaranteed to + // be null after a std::move. So the check only flags accesses that would + // dereference the pointer. { std::unique_ptr<A> ptr; std::move(ptr); ptr.get(); + static_cast<bool>(ptr); + *ptr; + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved + // CHECK-MESSAGES: [[@LINE-5]]:5: note: move occurred here + } + { + std::unique_ptr<A> ptr; + std::move(ptr); + ptr->foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } + { + std::unique_ptr<A> ptr; + std::move(ptr); + ptr[0]; + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here } { std::shared_ptr<A> ptr; std::move(ptr); ptr.get(); + static_cast<bool>(ptr); + *ptr; + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved + // CHECK-MESSAGES: [[@LINE-5]]:5: note: move occurred here + } + { + std::shared_ptr<A> ptr; + std::move(ptr); + ptr->foo(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here } - // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped - // in a typedef. + { + // std::weak_ptr<> cannot be dereferenced directly, so we only check that + // member functions may be called on it after a move. + std::weak_ptr<A> ptr; + std::move(ptr); + ptr.expired(); + } + // Make sure we recognize std::unique_ptr<> or std::shared_ptr<> if they're + // wrapped in a typedef. { typedef std::unique_ptr<A> PtrToA; PtrToA ptr; @@ -172,7 +225,8 @@ void uniquePtrAndSharedPtr() { std::move(ptr); ptr.get(); } - // And it's also true if the template argument is a little more involved. + // And we don't get confused if the template argument is a little more + // involved. { struct B { typedef A AnotherNameForA; @@ -181,6 +235,17 @@ void uniquePtrAndSharedPtr() { std::move(ptr); ptr.get(); } + // We don't give any special treatment to types that are called "unique_ptr" + // or "shared_ptr" but are not in the "::std" namespace. + { + struct unique_ptr { + void get(); + } ptr; + std::move(ptr); + ptr.get(); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved + // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here + } } // The check also works in member functions. @@ -795,6 +860,24 @@ void standardContainerAssignIsReinit() { } } +// Resetting the standard smart pointer types using reset() is treated as a +// re-initialization. (We don't test std::weak_ptr<> because it can't be +// dereferenced directly.) +void standardSmartPointerResetIsReinit() { + { + std::unique_ptr<A> ptr; + std::move(ptr); + ptr.reset(new A); + *ptr; + } + { + std::shared_ptr<A> ptr; + std::move(ptr); + ptr.reset(new A); + *ptr; + } +} + //////////////////////////////////////////////////////////////////////////////// // Tests related to order of evaluation within expressions _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
