Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, Charusso,
baloghadamsoftware, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, whisperity.
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
<https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D65378
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2058,12 +2058,6 @@
Callbacks.clear();
}
-BugReport::~BugReport() {
- while (!interestingSymbols.empty()) {
- popInterestingSymbolsAndRegions();
- }
-}
-
const Decl *BugReport::getDeclWithIssue() const {
if (DeclWithIssue)
return DeclWithIssue;
@@ -2101,10 +2095,10 @@
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 @@
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 @@
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 @@
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 @@
// Methods for BugReporter and subclasses.
//===----------------------------------------------------------------------===//
-BugReportEquivClass::~BugReportEquivClass() = default;
-
-GRBugReporter::~GRBugReporter() = default;
-
-BugReporterData::~BugReporterData() = default;
-
ExplodedGraph &GRBugReporter::getGraph() { return Eng.getGraph(); }
ProgramStateManager&
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -107,22 +107,19 @@
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 @@
/// 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 @@
: 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 @@
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 BugReporterData {
public:
- virtual ~BugReporterData();
+ virtual ~BugReporterData() = default;
virtual DiagnosticsEngine& getDiagnostic() = 0;
virtual ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() = 0;
@@ -526,7 +513,7 @@
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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
