[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
This revision was automatically updated to reflect the committed changes.
gribozavr marked an inline comment as done.
Closed by commit rL369504: Removed some dead code in BugReporter and related
files (authored by gribozavr, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66473?vs=216342=216345#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66473/new/
https://reviews.llvm.org/D66473
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
cfe/trunk/lib/StaticAnalyzer/Core/AnalysisManager.cpp
cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
cfe/trunk/unittests/StaticAnalyzer/Reusables.h
Index: cfe/trunk/unittests/StaticAnalyzer/Reusables.h
===
--- cfe/trunk/unittests/StaticAnalyzer/Reusables.h
+++ cfe/trunk/unittests/StaticAnalyzer/Reusables.h
@@ -58,7 +58,7 @@
ExprEngineConsumer(CompilerInstance )
: C(C), ChkMgr(C.getASTContext(), *C.getAnalyzerOpts()), CTU(C),
Consumers(),
-AMgr(C.getASTContext(), C.getDiagnostics(), Consumers,
+AMgr(C.getASTContext(), Consumers,
CreateRegionStoreManager, CreateRangeConstraintManager, ,
*C.getAnalyzerOpts()),
VisitedCallees(), FS(),
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -135,12 +135,6 @@
const StackFrameContext *OriginSFC;
public:
- /// Creates a visitor for every VarDecl inside a Stmt and registers it with
- /// the BugReport.
- static void registerStatementVarDecls(BugReport , const Stmt *S,
-bool EnableNullFPSuppression,
-TrackingKind TKind);
-
/// \param V We're searching for the store where \c R received this value.
/// \param R The region we're tracking.
/// \param TKind May limit the amount of notes added to the bug report.
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -735,8 +735,6 @@
PathPieces subPieces;
- bool containsEvent() const;
-
void flattenLocations() override {
PathDiagnosticSpotPiece::flattenLocations();
for (const auto : subPieces)
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -74,16 +74,6 @@
/// individual bug reports.
class BugReport : public llvm::ilist_node {
public:
- class NodeResolver {
-virtual void anchor();
-
- public:
-virtual ~NodeResolver() = default;
-
-virtual const ExplodedNode*
-getOriginalNode(const ExplodedNode *N) = 0;
- };
-
using ranges_iterator = const SourceRange *;
using VisitorList = SmallVector, 8>;
using visitor_iterator = VisitorList::iterator;
@@ -391,7 +381,6 @@
public:
virtual ~BugReporterData() = default;
- virtual DiagnosticsEngine& getDiagnostic() = 0;
virtual ArrayRef getPathDiagnosticConsumers() = 0;
virtual ASTContext () = 0;
virtual SourceManager () = 0;
@@ -404,11 +393,7 @@
///
/// The base class is used for generating path-insensitive
class BugReporter {
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
private:
- const Kind kind;
BugReporterData& D;
/// Generate and flush the diagnostics for the given bug report.
@@ -426,23 +411,13 @@
/// A vector of BugReports for tracking the allocated pointers and cleanup.
std::vector EQClassesVector;
-protected:
- BugReporter(BugReporterData& d, Kind k)
- : kind(k), D(d) {}
-
public:
- BugReporter(BugReporterData ) : kind(BasicBRKind), D(d) {}
+ BugReporter(BugReporterData ) : D(d) {}
virtual ~BugReporter();
/// Generate and flush diagnostics for all bug reports.
void FlushReports();
- Kind getKind() const { return kind; }
-
- DiagnosticsEngine& getDiagnostic() {
-return
[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
gribozavr marked 2 inline comments as done. gribozavr added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2343 InterExplodedGraphMap ForwardMap; - TrimmedGraph = OriginalGraph->trim(Nodes, , ); Szelethus wrote: > gribozavr wrote: > > NoQ wrote: > > > Btw these days we strongly suspect that the whole graph trimming thing is > > > useless and should be removed. > > TBH, I don't understand what this code is doing, I was just following the > > leads from dead code analysis :) > TL;DR: When creating a linear path from the root of the `ExplodedGraph` to a > given error node (a point at which a bug was emitted), we first trim be graph > of all nodes that do not lead to an error node, and then create the path from > that, instead of skipping the entire trimming process. > > This isn't that simple (though probably not that difficult either), so feel > free to leave it as it, the code is already much easier to read! Thanks for the explanation, I'll leave it as is in this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66473/new/ https://reviews.llvm.org/D66473 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
gribozavr updated this revision to Diff 216342.
gribozavr marked an inline comment as done.
gribozavr added a comment.
Updated a comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66473/new/
https://reviews.llvm.org/D66473
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
clang/unittests/StaticAnalyzer/Reusables.h
Index: clang/unittests/StaticAnalyzer/Reusables.h
===
--- clang/unittests/StaticAnalyzer/Reusables.h
+++ clang/unittests/StaticAnalyzer/Reusables.h
@@ -58,7 +58,7 @@
ExprEngineConsumer(CompilerInstance )
: C(C), ChkMgr(C.getASTContext(), *C.getAnalyzerOpts()), CTU(C),
Consumers(),
-AMgr(C.getASTContext(), C.getDiagnostics(), Consumers,
+AMgr(C.getASTContext(), Consumers,
CreateRegionStoreManager, CreateRangeConstraintManager, ,
*C.getAnalyzerOpts()),
VisitedCallees(), FS(),
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -311,9 +311,9 @@
checkerMgr = createCheckerManager(
*Ctx, *Opts, Plugins, CheckerRegistrationFns, PP.getDiagnostics());
-Mgr = std::make_unique(
-*Ctx, PP.getDiagnostics(), PathConsumers, CreateStoreMgr,
-CreateConstraintMgr, checkerMgr.get(), *Opts, Injector);
+Mgr = std::make_unique(*Ctx, PathConsumers, CreateStoreMgr,
+CreateConstraintMgr,
+checkerMgr.get(), *Opts, Injector);
}
/// Store the top level decls in the set to be processed later on.
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -53,17 +53,6 @@
using namespace clang;
using namespace ento;
-bool PathDiagnosticMacroPiece::containsEvent() const {
- for (const auto : subPieces) {
-if (isa(*P))
- return true;
-if (const auto *MP = dyn_cast(P.get()))
- if (MP->containsEvent())
-return true;
- }
- return false;
-}
-
static StringRef StripTrailingDots(StringRef s) {
for (StringRef::size_type i = s.size(); i != 0; --i)
if (s[i - 1] != '.')
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1166,41 +1166,6 @@
ID.AddBoolean(EnableNullFPSuppression);
}
-void FindLastStoreBRVisitor::registerStatementVarDecls(
-BugReport , const Stmt *S, bool EnableNullFPSuppression,
-TrackingKind TKind) {
-
- const ExplodedNode *N = BR.getErrorNode();
- std::deque WorkList;
- WorkList.push_back(S);
-
- while (!WorkList.empty()) {
-const Stmt *Head = WorkList.front();
-WorkList.pop_front();
-
-ProgramStateManager = N->getState()->getStateManager();
-
-if (const auto *DR = dyn_cast(Head)) {
- if (const auto *VD = dyn_cast(DR->getDecl())) {
-const VarRegion *R =
-StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());
-
-// What did we load?
-SVal V = N->getSVal(S);
-
-if (V.getAs() || V.getAs()) {
- // Register a new visitor with the BugReport.
- BR.addVisitor(std::make_unique(
- V.castAs(), R, EnableNullFPSuppression, TKind));
-}
- }
-}
-
-for (const Stmt *SubStmt : Head->children())
- WorkList.push_back(SubStmt);
- }
-}
-
/// Returns true if \p N represents the DeclStmt declaring and initializing
/// \p VR.
static bool isInitializationOfVar(const ExplodedNode *N, const VarRegion *VR) {
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2049,8 +2049,6 @@
// Methods for BugReport and subclasses.
//===--===//
-void BugReport::NodeResolver::anchor() {}
-
void BugReport::addVisitor(std::unique_ptr visitor) {
[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
gribozavr marked an inline comment as done.
gribozavr added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
NoQ wrote:
> gribozavr wrote:
> > NoQ wrote:
> > > Hey, i just added that! :D (well, renamed) (rC369320)
> > >
> > > I believe we do want a separation between a {bug report, bug reporter}
> > > classes that's only suitable for path-insensitive reports (which would
> > > live in libAnalysis and we could handle them to clang-tidy while still
> > > being able to compile it without CLANG_ENABLE_STATIC_ANALYZER) and all
> > > the path-sensitive report logic that is pretty huge but only Static
> > > Analyzer needs it. For that purpose we'd better leave this in. WDYT? See
> > > also D66460.
> > >
> > > Should i ask on the mailing list whether you're willing to sacrifice
> > > building clang-tidy without CLANG_ENABLE_STATIC_ANALYZER in order to
> > > transition to BugReporter? Cause i thought it was obvious that it's not a
> > > great idea, even if it causes me to do a bit of cleanup work on my end.
> > >
> > > That said, i'm surprised that it's deadcode, i.e. that nobody ever
> > > dyn_casts bug reporters, even though we already have two bug reporter
> > > classes. Maybe we can indeed remove this facility.
> > > I believe we do want a separation between a {bug report, bug reporter}
> > > classes [...]
> >
> > Yes, the separation is nice.
> >
> > > For that purpose we'd better leave this in.
> >
> > `Kind` is only needed for dynamic casting between different bug reporters.
> > I'm not sure that is useful in practice (case in point -- the `classof` is
> > not used today), specifically because the client that produces diagnostics
> > will need to work with a bug reporter of the correct kind. If a
> > path-sensitive client is handed a pointer to the base class, `BugReporter`,
> > would it try to `dyn_cast` it to the derived class?.. what if it fails?..
> >
> > Basically, I don't understand why one would want dynamic casting for these
> > classes. I totally agree with the separation though.
> >
> > > Should i ask on the mailing list whether you're willing to sacrifice
> > > building clang-tidy without CLANG_ENABLE_STATIC_ANALYZER in order to
> > > transition to BugReporter?
> >
> > I personally don't mind CLANG_ENABLE_STATIC_ANALYZER going away completely
> > (I have a fast machine and use a build system with strong caching),
> > however, there are other people who are a lot more sensitive to build time,
> > and for whom it might be important.
> > I personally don't mind CLANG_ENABLE_STATIC_ANALYZER going away completely
> > (I have a fast machine and use a build system with strong caching),
> > however, there are other people who are a lot more sensitive to build time,
> > and for whom it might be important.
>
> I think for clang it's mostly about binary size; people occasionally want
> compact clangs.
>
> > I'm not sure that is useful in practice (case in point -- the classof is
> > not used today), specifically because the client that produces diagnostics
> > will need to work with a bug reporter of the correct kind.
>
> Yeah, i agree. I'll add it back if i ever need it.
>
> I think for clang it's mostly about binary size; people occasionally want
> compact clangs.
That's true -- some people also want to have a small clang binary; however you
asked about clang-tidy without CLANG_ENABLE_STATIC_ANALYZER :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66473/new/
https://reviews.llvm.org/D66473
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
gribozavr wrote:
> NoQ wrote:
> > Hey, i just added that! :D (well, renamed) (rC369320)
> >
> > I believe we do want a separation between a {bug report, bug reporter}
> > classes that's only suitable for path-insensitive reports (which would live
> > in libAnalysis and we could handle them to clang-tidy while still being
> > able to compile it without CLANG_ENABLE_STATIC_ANALYZER) and all the
> > path-sensitive report logic that is pretty huge but only Static Analyzer
> > needs it. For that purpose we'd better leave this in. WDYT? See also D66460.
> >
> > Should i ask on the mailing list whether you're willing to sacrifice
> > building clang-tidy without CLANG_ENABLE_STATIC_ANALYZER in order to
> > transition to BugReporter? Cause i thought it was obvious that it's not a
> > great idea, even if it causes me to do a bit of cleanup work on my end.
> >
> > That said, i'm surprised that it's deadcode, i.e. that nobody ever
> > dyn_casts bug reporters, even though we already have two bug reporter
> > classes. Maybe we can indeed remove this facility.
> > I believe we do want a separation between a {bug report, bug reporter}
> > classes [...]
>
> Yes, the separation is nice.
>
> > For that purpose we'd better leave this in.
>
> `Kind` is only needed for dynamic casting between different bug reporters.
> I'm not sure that is useful in practice (case in point -- the `classof` is
> not used today), specifically because the client that produces diagnostics
> will need to work with a bug reporter of the correct kind. If a
> path-sensitive client is handed a pointer to the base class, `BugReporter`,
> would it try to `dyn_cast` it to the derived class?.. what if it fails?..
>
> Basically, I don't understand why one would want dynamic casting for these
> classes. I totally agree with the separation though.
>
> > Should i ask on the mailing list whether you're willing to sacrifice
> > building clang-tidy without CLANG_ENABLE_STATIC_ANALYZER in order to
> > transition to BugReporter?
>
> I personally don't mind CLANG_ENABLE_STATIC_ANALYZER going away completely (I
> have a fast machine and use a build system with strong caching), however,
> there are other people who are a lot more sensitive to build time, and for
> whom it might be important.
> I personally don't mind CLANG_ENABLE_STATIC_ANALYZER going away completely (I
> have a fast machine and use a build system with strong caching), however,
> there are other people who are a lot more sensitive to build time, and for
> whom it might be important.
I think for clang it's mostly about binary size; people occasionally want
compact clangs.
> I'm not sure that is useful in practice (case in point -- the classof is not
> used today), specifically because the client that produces diagnostics will
> need to work with a bug reporter of the correct kind.
Yeah, i agree. I'll add it back if i ever need it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66473/new/
https://reviews.llvm.org/D66473
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2343 InterExplodedGraphMap ForwardMap; - TrimmedGraph = OriginalGraph->trim(Nodes, , ); gribozavr wrote: > NoQ wrote: > > Btw these days we strongly suspect that the whole graph trimming thing is > > useless and should be removed. > TBH, I don't understand what this code is doing, I was just following the > leads from dead code analysis :) TL;DR: When creating a linear path from the root of the `ExplodedGraph` to a given error node (a point at which a bug was emitted), we first trim be graph of all nodes that do not lead to an error node, and then create the path from that, instead of skipping the entire trimming process. This isn't that simple (though probably not that difficult either), so feel free to leave it as it, the code is already much easier to read! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66473/new/ https://reviews.llvm.org/D66473 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
Szelethus added a comment.
No way the entire `NodeResolver` is dead code! I spent so much time trying to
understand it! I mean, now that I think about it, we literally deep copy every
`ExplodedNode`, so why would we need the mapping to the original, right?
Wow. Thank you so much for clearing this out.
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2263-2264
/// A wrapper around an ExplodedGraph that contains a single path from the root
/// to the error node, and a map that maps the nodes in this path to the ones
in
/// the original ExplodedGraph.
class BugPathInfo {
Let's keep the comment up to date as well :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66473/new/
https://reviews.llvm.org/D66473
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
