[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 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