[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, &ForwardMap, &InverseMap); 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, &ForwardMap, &InverseMap); 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 &BR, 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(C), ChkMgr(C.getASTContext(), *C.getAnalyzerOpts()), CTU(C), Consumers(), -AMgr(C.getASTContext(), C.getDiagnostics(), Consumers, +AMgr(C.getASTContext(), Consumers, CreateRegionStoreManager, CreateRangeConstraintManager, &ChkMgr, *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 &P : 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 &BR, 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 &StateMgr = 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