[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Thanks a lot, this is almost ready!
I think it should be good to go after this round of nitpicks.



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:168
 
-  virtual void EndPath(ProgramStateRef state) {}
-
----------------
>From a brief inspection this indeed seems dead code. However, this removal should be moved into a separate revision (sorry!)


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:680
-    ConstraintMgr->EndPath(St);
-  }
 };
----------------
Same: should be moved into a separate revision, same as the other removal.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1853
+
+    const auto *Cond = cast<Expr>(PS->getStmt());
+    auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);
----------------
How do we know that it's always an `Expr`?


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1854
+    const auto *Cond = cast<Expr>(PS->getStmt());
+    auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);
+    if (!Piece)
----------------
`/*tookTrue=*/tag == tag.first`


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1970
 
+bool ConditionBRVisitor::insertOrUpdateConstraintMessage(const Stmt *Cond,
+                                                         StringRef Message) {
----------------
1. What about the refactoring I have previously suggested twice?
2. I know you have started with that -- and sorry for spurious changes -- but I also think your original name of `constraintsChanged` is better.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2261
   PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
-  StateMgr.EndPath(Pred->getState());
 
----------------
Should be moved together with other two removals.


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:29
 
+/// Increments the number of times this state is referenced.
 void ProgramStateRetain(const ProgramState *state) {
----------------
While this minor formatting is correct, it's better to remove it to simplify future archaeology.


https://reviews.llvm.org/D53076