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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
