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