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
