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

[Szelethus]

Summary

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.

[NoQ]

I dunno why rC162028 was doing this. That's some advanced archeology down there.

[xazax.hun]

LGTM! I also do not see much value in the old code at this point. I would expect PathDiagnosticConsumers to be independent of the interesting symbols/regions.

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

protected:
   PathDiagnosticLocation UniqueingLocation;
   const Decl *UniqueingDecl;
 
   const ExplodedNode *ErrorNode = nullptr;
   SmallVector<SourceRange, 4> Ranges;
   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".
   llvm::SmallSet<const LocationContext *, 2> InterestingLocationContexts;
 
   /// A set of custom visitors which generate "event" diagnostics at
   /// interesting points in the path.
   VisitorList Callbacks;
protected:
   ///
   /// \sa markInvalid
   /// \sa removeInvalidation
   llvm::SmallSet<InvalidationRecord, 4> Invalidations;
 
   /// 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) {}
 
   BugReport(const BugType& bt, StringRef shortDesc, StringRef desc,
             const ExplodedNode *errornode)
       : BT(bt), ShortDescription(shortDesc), Description(desc),
         ErrorNode(errornode) {}
 
   BugReport(const BugType &bt, StringRef desc, PathDiagnosticLocation l)
       : BT(bt), Description(desc), Location(l) {}
 
   /// Create a BugReport with a custom uniqueing location.
   ///
   /// The reports that have the same report location, description, bug type, and
   /// ranges are uniqued - only one of the equivalent reports will be presented
   /// to the user. This method allows to rest the location which should be used
   /// for uniquing reports. For example, memory leaks checker, could set this to
   /// the allocation site, rather then the location where the bug is reported.
   BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode,
             PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique)
       : 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; }
 
   /// True when the report has an execution path associated with it.
   ///
   /// A report is said to be path-sensitive if it was thrown against a
   /// particular exploded node in the path-sensitive analysis graph.
class BugReportEquivClass : public llvm::FoldingSetNode {
   llvm::ilist<BugReport> Reports;
 
   void AddReport(std::unique_ptr<BugReport> R) {
     Reports.push_back(R.release());
   }
 
 public:
   BugReportEquivClass(std::unique_ptr<BugReport> R) { AddReport(std::move(R)); }
-  ~BugReportEquivClass();
 
   void Profile(llvm::FoldingSetNodeID& ID) const {
     assert(!Reports.empty());
     Reports.front().Profile(ID);
   }
 
   using iterator = llvm::ilist<BugReport>::iterator;
   using const_iterator = llvm::ilist<BugReport>::const_iterator;
 
   iterator begin() { return Reports.begin(); }
   iterator end() { return Reports.end(); }
 
   const_iterator begin() const { return Reports.begin(); }
   const_iterator end() const { return Reports.end(); }
 };
 
 //===----------------------------------------------------------------------===//
 // BugReporter and friends.
 //===----------------------------------------------------------------------===//
 
 class BugReporterData {
 public:
-  virtual ~BugReporterData();
+  virtual ~BugReporterData() = default;
 
   virtual DiagnosticsEngine& getDiagnostic() = 0;
   virtual ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() = 0;
   virtual ASTContext &getASTContext() = 0;
   virtual SourceManager &getSourceManager() = 0;
   virtual AnalyzerOptions &getAnalyzerOptions() = 0;
 };
 

 /// GRBugReporter is used for generating path-sensitive reports.
 class GRBugReporter : public BugReporter {
   ExprEngine& Eng;
 
 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.
   ExplodedGraph &getGraph();
 
   /// getStateManager - Return the state manager used by the analysis
   ///  engine.
   ProgramStateManager &getStateManager();

cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp

void BugReport::addVisitor(std::unique_ptr<BugReporterVisitor> visitor) {
 
   Callbacks.push_back(std::move(visitor));
 }
 
 void BugReport::clearVisitors() {
   Callbacks.clear();
 }
 
-BugReport::~BugReport() {
-  while (!interestingSymbols.empty()) {
-    popInterestingSymbolsAndRegions();
-  }
-}
-
 const Decl *BugReport::getDeclWithIssue() const {
   if (DeclWithIssue)
     return DeclWithIssue;
 
   const ExplodedNode *N = getErrorNode();
   if (!N)
     return nullptr;
 
for (SourceRange range : Ranges) {
     hash.AddInteger(range.getEnd().getRawEncoding());
   }
 }
 
 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) {
   markInteresting(V.getAsRegion());
   markInteresting(V.getAsSymbol());
 }
 
 void BugReport::markInteresting(const LocationContext *LC) {
   if (!LC)
     return;
   InterestingLocationContexts.insert(LC);
 }
 
 bool BugReport::isInteresting(SVal V) {
   return isInteresting(V.getAsRegion()) || isInteresting(V.getAsSymbol());
 }
 
 bool BugReport::isInteresting(SymbolRef sym) {
   if (!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;
 }
 
 bool BugReport::isInteresting(const LocationContext *LC) {
   if (!LC)
     return false;
   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;
 
   ProgramPoint ProgP = ErrorNode->getLocation();
   const Stmt *S = nullptr;
 
   if (Optional<BlockEntrance> BE = ProgP.getAs<BlockEntrance>()) {
PathDiagnosticLocation BugReport::getLocation(const SourceManager &SM) const {
   assert(Location.isValid());
   return Location;
 }
 
 //===----------------------------------------------------------------------===//
 // Methods for BugReporter and subclasses.
 //===----------------------------------------------------------------------===//
 
-BugReportEquivClass::~BugReportEquivClass() = default;
-
-GRBugReporter::~GRBugReporter() = default;
-
-BugReporterData::~BugReporterData() = default;
-
 ExplodedGraph &GRBugReporter::getGraph() { return Eng.getGraph(); }
 
 ProgramStateManager&
 GRBugReporter::getStateManager() { return Eng.getStateManager(); }
 
 BugReporter::~BugReporter() {
   FlushReports();