Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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! |
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h | ||
---|---|---|
408 ↗ | (On Diff #216137) |
Yes, the separation is nice.
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.
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 :) |
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 :) |
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! |
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.
Yeah, i agree. I'll add it back if i ever need it. |
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h | ||
---|---|---|
408 ↗ | (On Diff #216137) |
That's true -- some people also want to have a small clang binary; however you asked about clang-tidy without CLANG_ENABLE_STATIC_ANALYZER :) |
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | ||
---|---|---|
2343 ↗ | (On Diff #216137) | Thanks for the explanation, I'll leave it as is in this patch. |