[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Artem,
The overall idea is good but I have some remarks inline.



================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:265
 
+void MoveChecker::checkUse(ProgramStateRef State, const MemRegion *Region,
+                           const CXXRecordDecl *RD, MisuseKind MK,
----------------
I think that if the function is named "checkUse()", committing State changes is not what is really expected from it. Should we rename it or change the logic somehow? For example, return true if a report was emitted and add transition in the caller?


================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:272
+  if (!RS || isAnyBaseRegionReported(State, Region)
+          || isInMoveSafeContext(C.getLocationContext())) {
+    // Finalize changes made by the caller.
----------------
This formatting is different from what clang-format does.


================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:342
 
+  const CXXRecordDecl *RD = MethodDecl->getParent();
+
----------------
Should we move RD closer to its first use?


================
Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:490
+      if (CtorDec->isMoveConstructor())
+        checkUse(State, ArgRegion, RD, MK_Move, C);
+      else
----------------
```MisuseKind MK = CtorDecl->isMoveConstructor() ? MK_Move : MK_Copy;
checkUse(State, ArgRegion, RD, MK, C);```
?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55387/new/

https://reviews.llvm.org/D55387