[analyzer] MisusedMovedObjectChecker: More precise warning message

[szepet]

Summary

Added new enum in order to differentiate the warning messages on "misusing" into 3 categories: function calls, moving an object, copying an object. (At the moment the checker gives the same message in case of copying and moving.)

Additional test cases added as well.

Note: The dependency is only added for the reason not having conflict problems on the test cases.

[danielmarjamaki]

LGTM

[xazax.hun]

LGTM!

[zaks.anna]

Please, commit.

cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

checkRegionChanges(ProgramStateRef State,
                      const InvalidatedSymbols *Invalidated,
                      ArrayRef<const MemRegion *> ExplicitRegions,
                      ArrayRef<const MemRegion *> Regions,
                      const LocationContext *LCtx, const CallEvent *Call) const;
   void printState(raw_ostream &Out, ProgramStateRef State,
                   const char *NL, const char *Sep) const override;
 
 private:
+  enum MisuseKind {MK_FunCall, MK_Copy, MK_Move};
   class MovedBugVisitor : public BugReporterVisitorImpl<MovedBugVisitor> {
   public:
     MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {}
 
     void Profile(llvm::FoldingSetNodeID &ID) const override {
       static int X = 0;
       ID.AddPointer(&X);
       ID.AddPointer(Region);
     }
 
     std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                    const ExplodedNode *PrevN,
                                                    BugReporterContext &BRC,
                                                    BugReport &BR) override;
 
   private:
     // The tracked region.
     const MemRegion *Region;
     bool Found;
   };
 
   mutable std::unique_ptr<BugType> BT;
   ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call,
-                          CheckerContext &C, bool isCopy) const;
+                          CheckerContext &C, MisuseKind MK) const;
   bool isInMoveSafeContext(const LocationContext *LC) const;
   bool isStateResetMethod(const CXXMethodDecl *MethodDec) const;
   bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const;
   const ExplodedNode *getMoveLocation(const ExplodedNode *N,
                                       const MemRegion *Region,
                                       CheckerContext &C) const;
 };
 } // end anonymous namespace
while (N) {
     N = N->pred_empty() ? nullptr : *(N->pred_begin());
   }
   return MoveNode;
 }
 
 ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region,
                                                    const CallEvent &Call,
                                                    CheckerContext &C,
-                                                   bool isCopy = false) const {
+                                                   MisuseKind MK) const {
   if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     if (!BT)
       BT.reset(new BugType(this, "Usage of a 'moved-from' object",
                            "C++ move semantics"));
 
     // Uniqueing report to the same object.
     PathDiagnosticLocation LocUsedForUniqueing;
     const ExplodedNode *MoveNode = getMoveLocation(N, Region, C);
 
     if (const Stmt *MoveStmt = PathDiagnosticLocation::getStmt(MoveNode))
       LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
           MoveStmt, C.getSourceManager(), MoveNode->getLocationContext());
 
     // Creating the error message.
     std::string ErrorMessage;
-    if (isCopy)
-      ErrorMessage = "Copying a 'moved-from' object";
-    else
+    switch(MK) {
+      case MK_FunCall:
         ErrorMessage = "Method call on a 'moved-from' object";
+        break;
+      case MK_Copy:
+        ErrorMessage = "Copying a 'moved-from' object";
+        break;
+      case MK_Move:
+        ErrorMessage = "Moving a 'moved-from' object";
+        break;
+    }
     if (const auto DecReg = Region->getAs<DeclRegion>()) {
       const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl());
       ErrorMessage += " '" + RegionDecl->getNameAsString() + "'";
     }
 
     auto R =
         llvm::make_unique<BugReport>(*BT, ErrorMessage, N, LocUsedForUniqueing,
                                      MoveNode->getLocationContext()->getDecl());
if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) {
     State = removeFromState(State, CC->getCXXThisVal().getAsRegion());
     auto CtorDec = CC->getDecl();
     // Check for copying a moved-from object and report the bug.
     if (CtorDec && CtorDec->isCopyOrMoveConstructor()) {
       const MemRegion *ArgRegion = CC->getArgSVal(0).getAsRegion();
       const RegionState *ArgState = State->get<TrackedRegionMap>(ArgRegion);
       if (ArgState && ArgState->isMoved()) {
         if (!isInMoveSafeContext(LC)) {
-          N = reportBug(ArgRegion, Call, C, /*isCopy=*/true);
+          if(CtorDec->isMoveConstructor())
+            N = reportBug(ArgRegion, Call, C, MK_Move);
+          else
+            N = reportBug(ArgRegion, Call, C, MK_Copy);
           State = State->set<TrackedRegionMap>(ArgRegion,
                                                RegionState::getReported());
         }
       }
     }
     C.addTransition(State, N);
     return;
   }
void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
   if (OperatorEq) {
     State = removeFromState(State, ThisRegion);
     if (MethodDecl->isCopyAssignmentOperator() ||
         MethodDecl->isMoveAssignmentOperator()) {
       const RegionState *ArgState =
           State->get<TrackedRegionMap>(IC->getArgSVal(0).getAsRegion());
       if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) {
         const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion();
-        N = reportBug(ArgRegion, Call, C, /*isCopy=*/true);
+        if(MethodDecl->isMoveAssignmentOperator())
+          N = reportBug(ArgRegion, Call, C, MK_Move);
+        else
+          N = reportBug(ArgRegion, Call, C, MK_Copy);
         State =
             State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported());
       }
     }
     C.addTransition(State, N);
     return;
   }
 
void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
 
   // Don't report it in case if any base region is already reported
   if (isAnyBaseRegionReported(State, ThisRegion))
     return;
 
   if (isInMoveSafeContext(LC))
     return;
 
-  N = reportBug(ThisRegion, Call, C);
+  N = reportBug(ThisRegion, Call, C, MK_FunCall);
   State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());
   C.addTransition(State, N);
 }
 
 void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();

cfe/trunk/test/Analysis/MisusedMovedObject.cpp

 
 } // namespace std
 
 class B {
 public:
   B() = default;
   B(const B &) = default;
   B(B &&) = default;
+  B& operator=(const B &q) = default;
   void operator=(B &&b) {
     return;
   }
   void foo() { return; }
 };
 
 class A {
   int i;
A(A *a) {
     moveconstruct(std::move(*a));
   }
   A(const A &other) : i(other.i), d(other.d), b(other.b) {}
   A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}}
   }
   A(A &&other, char *k) {
     moveconstruct(std::move(other));
   }
+  void operator=(const A &other) {
+    i = other.i;
+    d = other.d;
+    b = other.b;
+    return;
+  }
   void operator=(A &&other) {
     moveconstruct(std::move(other));
     return;
   }
   int getI() { return i; }
   int foo() const;
   void bar() const;
   void reset();
void constCopyOrMoveCall(const A a) {
   a.foo();
 }
 
 void copyOrMoveCall(A a) {
   a.foo();
 }
 
 void simpleMoveCtorTest() {
+  {
     A a;
-  A b;
-  b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
-  a.foo();          // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
+    A b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+    a.foo();            // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
   }
+  {
+    A a;
+    A b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+    b = a;              // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}}
+  }
+  {
+    A a;
+    A b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+    b = std::move(a);   // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}}
+  }
+}
 
 void simpleMoveAssignementTest() {
+  {
     A a;
     A b;
     b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
     a.foo();          // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
   }
+  {
+    A a;
+    A b;
+    b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+    A c(a);           // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}}
+  }
+  {
+    A a;
+    A b;
+    b = std::move(a);  // expected-note {{'a' became 'moved-from' here}}
+    A c(std::move(a)); // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}}
+  }
+}
 
 void moveInInitListTest() {
   struct S {
     A a;
   };
   A a;
   S s{std::move(a)}; // expected-note {{'a' became 'moved-from' here}}
   a.foo();           // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
for (int i = 0; i < 2; i++) { // expected-note {{Loop condition is true.}}
       //expected-note@-1 {{Loop condition is true.  Entering loop body}}
       //expected-note@-2 {{Loop condition is false. Execution jumps to the end of the function}}
       copyOrMoveCall(a);          // no-warning
     }
   }
   {
     A a;
     for (int i = 0; i < bignum(); i++) { // expected-note {{Loop condition is true.  Entering loop body}} expected-note {{Loop condition is true.  Entering loop body}}
-      constCopyOrMoveCall(std::move(a)); // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}}
+      constCopyOrMoveCall(std::move(a)); // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}}
       // expected-note@-1 {{'a' became 'moved-from' here}}
     }
   }
 
   // Don't warn if we return after the move.
   {
     A a;
     for (int i = 0; i < 3; ++i) {
void differentBranchesTest(int i) {
   // A variation on the theme above.
   {
     A a;
     a.foo() > 0 ? a.foo() : A(std::move(a)).foo(); // expected-note {{Assuming the condition is false}} expected-note {{'?' condition is false}}
   }
   // Same thing, but with a switch statement.
   {
     A a, b;
-    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 451}}
+    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 483}}
     case 1:
       b = std::move(a); // no-warning
       break;            // expected-note {{Execution jumps to the end of the function}}
     case 2:
       a.foo(); // no-warning
       break;
     }
   }
   // However, if there's a fallthrough, we do warn.
   {
     A a, b;
-    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 463}}
+    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 495}}
     case 1:
       b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
     case 2:
       a.foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}}
       break;
     }
   }
 }