Page MenuHomePhabricator

[analyzer] Removed some dead code in BugReporter and related files
ClosedPublic

Authored by gribozavr on Aug 20 2019, 6:57 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

gribozavr created this revision.Aug 20 2019, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 6:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ added a comment.Aug 20 2019, 7:23 AM

Yay, that'll make my life a lot easier! I heard you have an automatic tool for this sort of stuff, right?

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
408 ↗(On Diff #216137)

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.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2343 ↗(On Diff #216137)

Btw these days we strongly suspect that the whole graph trimming thing is useless and should be removed.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1169 ↗(On Diff #216137)

Woohooo!

gribozavr marked 3 inline comments as done.Aug 20 2019, 11:41 AM
gribozavr added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
408 ↗(On Diff #216137)

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.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2343 ↗(On Diff #216137)

TBH, I don't understand what this code is doing, I was just following the leads from dead code analysis :)

Szelethus retitled this revision from Removed some dead code in BugReporter and related files to [analyzer] Removed some dead code in BugReporter and related files.Aug 20 2019, 11:43 AM

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.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2263–2264 ↗(On Diff #216137)

Let's keep the comment up to date as well :)

Szelethus added inline comments.Aug 20 2019, 11:55 AM
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2343 ↗(On Diff #216137)

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!

NoQ accepted this revision.Aug 20 2019, 1:43 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
408 ↗(On Diff #216137)

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.

This revision is now accepted and ready to land.Aug 20 2019, 1:43 PM
gribozavr marked an inline comment as done.Aug 20 2019, 1:55 PM
gribozavr added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
408 ↗(On Diff #216137)

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 :)

gribozavr updated this revision to Diff 216342.Aug 21 2019, 1:34 AM
gribozavr marked an inline comment as done.

Updated a comment.

gribozavr marked 2 inline comments as done.Aug 21 2019, 1:35 AM
gribozavr added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2343 ↗(On Diff #216137)

Thanks for the explanation, I'll leave it as is in this patch.

This revision was automatically updated to reflect the committed changes.
gribozavr marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 1:50 AM