george.karpenkov requested changes to this revision. george.karpenkov added inline comments. This revision now requires changes to proceed.
================ Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:80 typedef llvm::ImmutableMap<void*, void*> GenericDataMap; + typedef llvm::DenseMap<const Stmt *, StringRef> ConstraintMap; ---------------- Generally, `StringRef`'s shouldn't be stored in containers, as containers might outlive them. It might be fine in this case, but I would prefer to be on the safe side and just use `std::string` ================ Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:238 + /// message on that constraint being changed. + bool isChangedOrInsertConstraint(ConstraintMap &Constraints, const Stmt *Cond, + StringRef Message) const { ---------------- whisperity wrote: > `insertOrUpdateContraintMessage` > > and mention in the documentation that it returns whether or not the actual > insertion or update change took place If constraints is now a property of the visitor, shouldn't this method with the typedef above be moved to the visitor as well? ================ Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:244 + if (I == Constraints.end() || !Message.equals(I->second)) { + Constraints[Cond] = Message; + return true; ---------------- Isn't that equivalent to `Constraints.insert(make_pair(Cond, Message)).second` ? I think I have written that before. ================ Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2232 + const ExplodedNode *N, BugReport &BR) { + Constraints.clear(); +} ---------------- It does not seem necessary, because a new copy of the visitor is created for each new bug report. https://reviews.llvm.org/D53076 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits