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