[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. In D54438#1322266 , @NoQ wrote: > In D54438#1315953 , @Szelethus wrote: > > > - ✔️ There are in fact a variety of checkers that contain subcheckers like > > `MallocChecker`, which I think

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. In D54438#1315953 , @Szelethus wrote: > - ✔️ There are in fact a variety of checkers that contain subcheckers like > `MallocChecker`, which I think is all good, but the concept that if a > subchecker is enabled it only sort of regist

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. Okay. I was wrong, and you were right. `MallocChecker` seriously suffers from overcrowding. Especially with the addition of `InnerPointerChecker`, it should be split up, but doing that is very much avoidable for the purposes of this effort while still achieving every

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. Hmmm, I was wrong a little bit: I realized that if I tinker with with `initializeManager` and `getEnabledCheckers` just a bit more, register checkers straight away, don't need to collect them first, and this would make sure that dependencies are registered before the

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. Herald added a subscriber: gamesh411. In https://reviews.llvm.org/D54438#1297602, @NoQ wrote: > Hmm, makes sense. Maybe `static bool BlahBlahChecker::isApplicable(const > LangOpts &LO)` or something like that. > > Btw, maybe instead of default constructor and `->setup(

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. Hmm, makes sense. Maybe `static bool BlahBlahChecker::isApplicable(const LangOpts &LO)` or something like that. Btw, maybe instead of default constructor and `->setup(Args)` method we could stuff all of the `setup(Args)`'s arguments into the one-and-only constructor for th

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. Thanks you so much! One of the reasons why I love working on this is because the great feedback I get. I don't wanna seem annoyingly grateful, but it's been a very enjoyable experience so far. > This should not cause a warnings because it should be fine to compile >

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote: > - Some checkers do not register themselves in all cases[1], which should > probably be handled at a higher level, preferably in `Checkers.td`. Warnings > could be emitted when a checker is explicitly enabled bu

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus planned changes to this revision. Szelethus added a comment. Here's how I'm planning to approach this issue: - Split `MallocChecker` and `CStringChecker` up properly, introduce `MallocCheckerBase` and `CStringCheckerBase`. I'm a little unsure about how I'll do this, but I'll share my

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. Herald added a subscriber: baloghadamsoftware. Allow me to disagree again -- the reason why `CheckerRegistry` and the entire checker registration process is a mess is that suboptimal solutions were added instead of making the system do well on the long term. This is co

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Ok, so the point is to fix the current checker name problem, right? I guess let's land it then :) Code looks great. I'm thinking aloud in inline comments a little bit, but don't mind me. @rnkovacs

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. Actually, no. The main problem here is that `InnerPointerChecker` doesn't only want to register `MallocBase`, it needs to modify the checker object. In any case, either `MallocChecker.cpp` needs the definition of `InnerPointerChecker`, or vice versa. Sure, I could sep

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. In https://reviews.llvm.org/D54438#1296239, @NoQ wrote: > Mm, i don't understand. I mean, what prevents you from cutting it off even > earlier and completely omitting that part of the patch? Somebody will get to > this later in order to see how exactly does the separa

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. Mm, i don't understand. I mean, what prevents you from cutting it off even earlier and completely omitting that part of the patch? Somebody will get to this later in order to see how exactly does the separation needs to be performed. Repository: rC Clang https://review

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus added a comment. In https://reviews.llvm.org/D54438#1296155, @NoQ wrote: > Can we reduce this patch to simply introducing the dependency item in > `Checkers.td` and using it in, like, one place (eg., > `MallocChecker`/`CStringChecker` inter-op) and discuss the rest of the stuff > lat

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. Hmm. All intentions in this patch are great and i love them! I think that the refactoring work in `MallocChecker` is a bit pre-mature; i don't have a clear picture of how the ideal `MallocChecker` should look like, and i'm not sure this patch moves us into the right direct

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

Szelethus created this revision. Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, jfb, mikhail.ramalho, a.sidorin, mgrang, szepet, whisperity, mgorny. This patch solves the problem I explained in an earlier revision[