[clang-tools-extra] r285842 - [clang-tidy] Extend misc-use-after-move to support unique_ptr and shared_ptr.

Martin Bohme via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 2 10:34:47 PDT 2016


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
 




More information about the cfe-commits mailing list