[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I looked at bug path with the Optional. I did not debug into the analyzer but my intuition is the following: `DefChainEnd` is interesting. `TerminatedPaths` is the control dependency of `DefChainEnd`. And there are probably other things that are the control and/or value dependency of `TerminatedPaths` . One trick we could try is to limit the number of dependencies we track. For example, we could try what happens if we only track the immediate control dependencies of the original interesting region/symbol and does not add more control dependency recursively.

In case we find limiting the number of dependencies useful, I could imagine a user-facing flag like `report-verbosity`. The default one could include only the notes we find useful after limiting the tracking. A detailed option could add all the notes from the transitive tracking of dependencies. A verbose option could remove path pruning as well. This is just random brainstorming :) First, we need to so whether non-transitive tracking it is actually improving the situation.

I am not sure about assuming `operator bool` being correct. I think we first could think about other tricks to limit the tracking (see my idea above) and if we fail I would only add such rules as a last resort.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:157
+  /// Conditions we're already tracking.
+  llvm::SmallPtrSet<const Expr *, 4> TrackedConditions;
+
----------------
Do we need this? I wonder if marking the result of the condition as interesting is sufficient.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1850
+
+    return const_cast<CFGBlock *>(Node->getLocationContext()
+        ->getAnalysisDeclContext()->getCFGStmtMap()->getBlock(S));
----------------
Why do we need a ptr to non-const CFGBlock?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1895
+
+  if (ControlDepTree.isControlDependency(OriginB, NB))
+    if (const Expr *Condition = getTerminatorCondition(NB))
----------------
Maybe this is the worng place to comment, but `isControlDependency` sounds a bit weird. How about `isControlDependenent`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883