Index: clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp @@ -463,6 +463,26 @@ }); } +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 *DeclRefs) { @@ -472,17 +492,33 @@ if (!S) continue; - SmallVector Matches = - match(findAll(declRefExpr(hasDeclaration(equalsNode(MovedVariable)), - unless(inDecltypeOrTemplateArg())) - .bind("declref")), - *S->getStmt(), *Context); + auto addDeclRefs = [this, Block, + DeclRefs](const ArrayRef Matches) { + for (const auto &Match : Matches) { + const auto *DeclRef = Match.getNodeAs("declref"); + const auto *Operator = Match.getNodeAs("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("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 @@ "::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 @@ // 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 @@ 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())) Index: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst @@ -75,10 +75,6 @@ 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 @@ 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``) is called on it. + If multiple uses occur after a move, only the first of these is flagged. Reinitialization @@ -172,6 +175,9 @@ ``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 Index: clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp +++ clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp @@ -9,12 +9,27 @@ 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 struct shared_ptr { shared_ptr(); T *get() const; + explicit operator bool() const; + void reset(T *ptr); + T &operator*() const; + T *operator->() const; +}; + +template +struct weak_ptr { + weak_ptr(); + bool expired() const; }; #define DECLARE_STANDARD_CONTAINER(name) \ @@ -146,20 +161,58 @@ // 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 ptr; std::move(ptr); ptr.get(); + static_cast(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 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 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 ptr; std::move(ptr); ptr.get(); + static_cast(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 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 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 PtrToA; PtrToA ptr; @@ -172,7 +225,8 @@ 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 @@ 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 @@ } } +// 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 ptr; + std::move(ptr); + ptr.reset(new A); + *ptr; + } + { + std::shared_ptr ptr; + std::move(ptr); + ptr.reset(new A); + *ptr; + } +} + //////////////////////////////////////////////////////////////////////////////// // Tests related to order of evaluation within expressions