[analyzer][NFC] Refactoring BugReporter.cpp P1.: Store interesting symbols/regions...

The goal of this refactoring effort was to better understand how interestingness
was propagated in BugReporter.cpp, which eventually turned out to be a dead end,
but with such a twist, I wouldn't even want to spoil it ahead of time. However,
I did get to learn a lot about how things are working in there.

In these series of patches, as well as cleaning up the code big time, I invite
you to study how BugReporter.cpp operates, and discuss how we could design this
file to reduce the horrible mess that it is.

This patch reverts a great part of rC162028, which holds the title "Allow
multiple PathDiagnosticConsumers to be used with a BugReporter at the same
time.". This, however doesn't imply that there's any need for multiple "layers"
or stacks of interesting symbols and regions, quite the contrary, I would argue
that we would like to generate the same amount of information for all output
types, and only process them differently.

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

llvm-svn: 368689

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
index d30ad19b20f85343cd3a008321f062da6834cbf8..f99af2be2d4c83a71559d83aad25e430da3c337a 100644 (file)
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -107,22 +107,19 @@ protected:
   ExtraTextList ExtraText;
   NoteList Notes;
 
-  using Symbols = llvm::DenseSet<SymbolRef>;
-  using Regions = llvm::DenseSet<const MemRegion *>;
-
   /// A (stack of) a set of symbols that are registered with this
   /// report as being "interesting", and thus used to help decide which
   /// diagnostics to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
-  SmallVector<Symbols *, 2> interestingSymbols;
+  llvm::DenseSet<SymbolRef> InterestingSymbols;
 
   /// A (stack of) set of regions that are registered with this report as being
   /// "interesting", and thus used to help decide which diagnostics
   /// to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
-  SmallVector<Regions *, 2> interestingRegions;
+  llvm::DenseSet<const MemRegion *> InterestingRegions;
 
   /// A set of location contexts that correspoind to call sites which should be
   /// considered "interesting".
@@ -156,15 +153,6 @@ protected:
   /// Conditions we're already tracking.
   llvm::SmallSet<const ExplodedNode *, 4> TrackedConditions;
 
-private:
-  // Used internally by BugReporter.
-  Symbols &getInterestingSymbols();
-  Regions &getInterestingRegions();
-
-  void lazyInitializeInterestingSets();
-  void pushInterestingSymbolsAndRegions();
-  void popInterestingSymbolsAndRegions();
-
 public:
   BugReport(const BugType& bt, StringRef desc, const ExplodedNode *errornode)
       : BT(bt), Description(desc), ErrorNode(errornode) {}
@@ -189,7 +177,7 @@ public:
       : BT(bt), Description(desc), UniqueingLocation(LocationToUnique),
         UniqueingDecl(DeclToUnique), ErrorNode(errornode) {}
 
-  virtual ~BugReport();
+  virtual ~BugReport() = default;
 
   const BugType& getBugType() const { return BT; }
   //BugType& getBugType() { return BT; }
@@ -381,7 +369,6 @@ class BugReportEquivClass : public llvm::FoldingSetNode {
 
 public:
   BugReportEquivClass(std::unique_ptr<BugReport> R) { AddReport(std::move(R)); }
-  ~BugReportEquivClass();
 
   void Profile(llvm::FoldingSetNodeID& ID) const {
     assert(!Reports.empty());
@@ -404,7 +391,7 @@ public:
 
 class BugReporterData {
 public:
-  virtual ~BugReporterData();
+  virtual ~BugReporterData() = default;
 
   virtual DiagnosticsEngine& getDiagnostic() = 0;
   virtual ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() = 0;
@@ -526,7 +513,7 @@ public:
   GRBugReporter(BugReporterData& d, ExprEngine& eng)
       : BugReporter(d, GRBugReporterKind), Eng(eng) {}
 
-  ~GRBugReporter() override;
+  ~GRBugReporter() override = default;;
 
   /// getGraph - Get the exploded graph created by the analysis engine
   ///  for the analyzed method or function.

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index e5a0794f10e2ce32aec4bb19161956e03e542e6c..262c6a10797fc756415cfef4a87275604dcc51c0 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2058,12 +2058,6 @@ void BugReport::clearVisitors() {
   Callbacks.clear();
 }
 
-BugReport::~BugReport() {
-  while (!interestingSymbols.empty()) {
-    popInterestingSymbolsAndRegions();
-  }
-}
-
 const Decl *BugReport::getDeclWithIssue() const {
   if (DeclWithIssue)
     return DeclWithIssue;
@@ -2101,10 +2095,10 @@ void BugReport::markInteresting(SymbolRef sym) {
   if (!sym)
     return;
 
-  getInterestingSymbols().insert(sym);
+  InterestingSymbols.insert(sym);
 
   if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
-    getInterestingRegions().insert(meta->getRegion());
+    InterestingRegions.insert(meta->getRegion());
 }
 
 void BugReport::markInteresting(const MemRegion *R) {
@@ -2112,10 +2106,10 @@ void BugReport::markInteresting(const MemRegion *R) {
     return;
 
   R = R->getBaseRegion();
-  getInterestingRegions().insert(R);
+  InterestingRegions.insert(R);
 
   if (const auto *SR = dyn_cast<SymbolicRegion>(R))
-    getInterestingSymbols().insert(SR->getSymbol());
+    InterestingSymbols.insert(SR->getSymbol());
 }
 
 void BugReport::markInteresting(SVal V) {
@@ -2138,18 +2132,18 @@ bool BugReport::isInteresting(SymbolRef sym) {
     return false;
   // We don't currently consider metadata symbols to be interesting
   // even if we know their region is interesting. Is that correct behavior?
-  return getInterestingSymbols().count(sym);
+  return InterestingSymbols.count(sym);
 }
 
 bool BugReport::isInteresting(const MemRegion *R) {
   if (!R)
     return false;
   R = R->getBaseRegion();
-  bool b = getInterestingRegions().count(R);
+  bool b = InterestingRegions.count(R);
   if (b)
     return true;
   if (const auto *SR = dyn_cast<SymbolicRegion>(R))
-    return getInterestingSymbols().count(SR->getSymbol());
+    return InterestingSymbols.count(SR->getSymbol());
   return false;
 }
 
@@ -2159,33 +2153,6 @@ bool BugReport::isInteresting(const LocationContext *LC) {
   return InterestingLocationContexts.count(LC);
 }
 
-void BugReport::lazyInitializeInterestingSets() {
-  if (interestingSymbols.empty()) {
-    interestingSymbols.push_back(new Symbols());
-    interestingRegions.push_back(new Regions());
-  }
-}
-
-BugReport::Symbols &BugReport::getInterestingSymbols() {
-  lazyInitializeInterestingSets();
-  return *interestingSymbols.back();
-}
-
-BugReport::Regions &BugReport::getInterestingRegions() {
-  lazyInitializeInterestingSets();
-  return *interestingRegions.back();
-}
-
-void BugReport::pushInterestingSymbolsAndRegions() {
-  interestingSymbols.push_back(new Symbols(getInterestingSymbols()));
-  interestingRegions.push_back(new Regions(getInterestingRegions()));
-}
-
-void BugReport::popInterestingSymbolsAndRegions() {
-  delete interestingSymbols.pop_back_val();
-  delete interestingRegions.pop_back_val();
-}
-
 const Stmt *BugReport::getStmt() const {
   if (!ErrorNode)
     return nullptr;
@@ -2236,12 +2203,6 @@ PathDiagnosticLocation BugReport::getLocation(const SourceManager &SM) const {
 // Methods for BugReporter and subclasses.
 //===----------------------------------------------------------------------===//
 
-BugReportEquivClass::~BugReportEquivClass() = default;
-
-GRBugReporter::~GRBugReporter() = default;
-
-BugReporterData::~BugReporterData() = default;
-
 ExplodedGraph &GRBugReporter::getGraph() { return Eng.getGraph(); }
 
 ProgramStateManager&