NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408
-public:
- enum Kind { BasicBRKind, PathSensitiveBRKind };
-
----------------
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.
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
