[PATCH] D66473: Removed some dead code in BugReporter and related files
gribozavr marked 3 inline comments as done.
gribozavr added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
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.
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2343
InterExplodedGraphMap ForwardMap;
- TrimmedGraph = OriginalGraph->trim(Nodes, , );
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 :)
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: Removed some dead code in BugReporter and related files
NoQ added a comment.
Yay, that'll make my life a lot easier! I heard you have an automatic tool for
this sort of stuff, right?
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
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.
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2343
InterExplodedGraphMap ForwardMap;
- TrimmedGraph = OriginalGraph->trim(Nodes, , );
Btw these days we strongly suspect that the whole graph trimming thing is
useless and should be removed.
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1169
-void FindLastStoreBRVisitor::registerStatementVarDecls(
-BugReport , const Stmt *S, bool EnableNullFPSuppression,
Woohooo!
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: Removed some dead code in BugReporter and related files
gribozavr created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
gribozavr added a reviewer: NoQ.
Repository:
rG LLVM Github Monorepo
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) {
if (!visitor)
return;
@@ -2218,10 +2216,6 @@
return
