[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files
Dmitri Gribenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 20 13:55:21 PDT 2019
gribozavr marked an inline comment as done.
gribozavr added inline comments.
================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
----------------
NoQ wrote:
> gribozavr wrote:
> > 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.
> > 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.
>
> 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 :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66473/new/
https://reviews.llvm.org/D66473
More information about the cfe-commits
mailing list