[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

Tue Jan 22 12:41:57 PST 2019

Szelethus added a comment.

Poor wording on my end, sorry about that. Let me clarify.

In D55400#1366684 <https://reviews.llvm.org/D55400#1366684>, @george.karpenkov wrote:

> > Deal with the consequences of this, and just correct all plist files to now refer to the new base checker.
>
> What does it mean?

When a report is emitted in the plist output, it'll also that which checker is "responsible" for that warning. I vaguely remember reading that this isn't used that much in your application, but we use this information extensively here.

Now, what I'd propose, is register a new, `osx.RetainCountBase` checker, and make both of the already existing checkers depend on that. Long story short, this will make the name of the checker that is listed in that plist entry `osx.RetainCountBase`, as opposed to `osx.cocoa.RetainCount`, but this doesn't sound that illogical, considering that both of those checkers emit the same checker name anyways (meaning, that if one report originates from `osx.OSObjectRetainCount` and one from `osx.cocoa.RetainCount`, both reports will be listed as they originated from `osx.cocoa.RetainCount`).

> 
> 
>> Each time a report is emitted from these checkers, create a ProgramPointTag, and "manually" make sure the emitted checker name doesn't change.
> 
> I'm not sure what do you propose exactly, but sounds pretty bad.

It does. I think `IvarInvalidation` uses something like that, and it's both unsustainable and very ugly.

>> Rename osx.cocoa.RetainCount to something else. This one is clearly nonsensical, but let's put it here for the sake of completeness.
> 
> I don't think we can rename the old checker, as older Xcode versions have "osx.cocoa.RetainCount" hardcoded in them.

Yea, this one makes little sense. Should've just left this one out really.

> TBH I'm not really sold on the way we "bundle" multiple checkers (from the tablegen) into a single class. [...] essentially the checker name is defined by the class which was registered by the tablegen first (which should not even be visible, and should be an internal implementation detail)
>  Do you have better proposals on how, conceptually, grouped checkers should be organized?

Yup, I've had the same thought before. I would argue strongly for registering them in the tblgen file, because a clear dependency graph can be easily established this way. What would be neat, however, is to also a new bit field to `Checker` in `CheckerBase.td`, which if set, will make the checker hidden in the `-analyzer-checker-help` list. There are many checkers that should be there anyways, implicit checker come to my mind first.

> For one, that means options don't work at all, and [...]

Hmmm, does this mess with options that bad? Could you please clarify?

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55400/new/

https://reviews.llvm.org/D55400