[analyzer][NFC] Refactoring BugReporter.cpp P1.: Store interesting symbols/regions in a simple set

Szelethus · Szelethus · commit b9bd6ebe1dca · 2019-08-13T13:09:48.000Z
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
@@ -107,22 +107,19 @@ class BugReport : public llvm::ilist_node<BugReport> {
   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 @@ class BugReport : public llvm::ilist_node<BugReport> {
   /// 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 @@ class BugReport : public llvm::ilist_node<BugReport> {
       : 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 @@ class BugReportEquivClass : public llvm::FoldingSetNode {
 
 class BugReporterData {
 public:
-  virtual ~BugReporterData();
+  virtual ~BugReporterData() = default;
 
   virtual DiagnosticsEngine& getDiagnostic() = 0;
   virtual ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() = 0;
@@ -526,7 +513,7 @@ class GRBugReporter : public BugReporter {
   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
@@ -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,21 +2095,21 @@ 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) {
   if (!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&
