[analyzer][NFC] Refactoring BugReporter.cpp P2.: Clean up the constru…

…ction of bug paths and finding a valid report

This patch refactors the utility functions and classes around the construction
of a bug path.

At a very high level, this consists of 3 steps:

* For all BugReports in the same BugReportEquivClass, collect all their error
nodes in a set. With that set, create a new, trimmed ExplodedGraph whose leafs
are all error nodes.
* Until a valid report is found, construct a bug path, which is yet another
ExplodedGraph, that is linear from a given error node to the root of the graph.
* Run all visitors on the constructed bug path. If in this process the report
got invalidated, start over from step 2.

Now, to the changes within this patch:

* Do not allow the invalidation of BugReports up to the point where the trimmed
graph is constructed. Checkers shouldn't add bug reports that are known to be
invalid, and should use visitors and argue about the entirety of the bug path if
needed.
* Do not calculate indices. I may be biased, but I personally find code like
this horrible. I'd like to point you to one of the comments in the original code:

SmallVector<const ExplodedNode *, 32> errorNodes;
for (const auto I : bugReports) {
  if (I->isValid()) {
    HasValid = true;
    errorNodes.push_back(I->getErrorNode());
  } else {
    // Keep the errorNodes list in sync with the bugReports list.
    errorNodes.push_back(nullptr);
  }
}

Not on my watch. Instead, use a far easier to follow trick: store a pointer to
the BugReport in question, not an index to it.

* Add range iterators to ExplodedGraph's successors and predecessors, and a
visitor range to BugReporter.
* Rename TrimmedGraph to BugPathGetter. Because that is what it has always been:
no sane graph type should store an iterator-like state, or have an interface not
exposing a single graph-like functionalities.
* Rename ReportGraph to BugPathInfo, because it is only a linear path with some
other context.
* Instead of having both and out and in parameter (which I think isn't ever
excusable unless we use the out-param for caching), return a record object with
descriptive getter methods.
* Where descriptive names weren't sufficient, compliment the code with comments.

Differential Revision: https://reviews.llvm.org/D65379

llvm-svn: 368694

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

@@ -87,6 +87,7 @@ class BugReport : public llvm::ilist_node<BugReport> {
   using ranges_iterator = const SourceRange *;
   using VisitorList = SmallVector<std::unique_ptr<BugReporterVisitor>, 8>;
   using visitor_iterator = VisitorList::iterator;
+  using visitor_range = llvm::iterator_range<visitor_iterator>;
   using ExtraTextList = SmallVector<StringRef, 2>;
   using NoteList = SmallVector<std::shared_ptr<PathDiagnosticNotePiece>, 4>;
 
@@ -339,6 +340,7 @@ class BugReport : public llvm::ilist_node<BugReport> {
   /// Iterators through the custom diagnostic visitors.
   visitor_iterator visitor_begin() { return Callbacks.begin(); }
   visitor_iterator visitor_end() { return Callbacks.end(); }
+  visitor_range visitors() { return {visitor_begin(), visitor_end()}; }
 
   /// Notes that the condition of the CFGBlock associated with \p Cond is
   /// being tracked.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h

@@ -219,29 +219,40 @@ class ExplodedNode : public llvm::FoldingSetNode {
 
   // Iterators over successor and predecessor vertices.
   using succ_iterator = ExplodedNode * const *;
+  using succ_range = llvm::iterator_range<succ_iterator>;
+
   using const_succ_iterator = const ExplodedNode * const *;
+  using const_succ_range = llvm::iterator_range<const_succ_iterator>;
+
   using pred_iterator = ExplodedNode * const *;
+  using pred_range = llvm::iterator_range<pred_iterator>;
+
   using const_pred_iterator = const ExplodedNode * const *;
+  using const_pred_range = llvm::iterator_range<const_pred_iterator>;
 
   pred_iterator pred_begin() { return Preds.begin(); }
   pred_iterator pred_end() { return Preds.end(); }
+  pred_range preds() { return {Preds.begin(), Preds.end()}; }
 
   const_pred_iterator pred_begin() const {
     return const_cast<ExplodedNode*>(this)->pred_begin();
   }
   const_pred_iterator pred_end() const {
     return const_cast<ExplodedNode*>(this)->pred_end();
   }
+  const_pred_range preds() const { return {Preds.begin(), Preds.end()}; }
 
   succ_iterator succ_begin() { return Succs.begin(); }
   succ_iterator succ_end() { return Succs.end(); }
+  succ_range succs() { return {Succs.begin(), Succs.end()}; }
 
   const_succ_iterator succ_begin() const {
     return const_cast<ExplodedNode*>(this)->succ_begin();
   }
   const_succ_iterator succ_end() const {
     return const_cast<ExplodedNode*>(this)->succ_end();
   }
+  const_succ_range succs() const { return {Succs.begin(), Succs.end()}; }
 
   int64_t getID(ExplodedGraph *G) const;