HomePhabricator

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

Authored by Szelethus on Aug 13 2019, 6:56 AM.

Description

[analyzer][NFC] Refactoring BugReporter.cpp P2.: Clean up the construction 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