Page MenuHomePhabricator

[analyzer][RetainCount] Tie diagnostics to osx.cocoa.RetainCount rather then RetainCountBase, for the most part
ClosedPublic

Authored by Szelethus on Apr 14 2020, 4:56 AM.

Details

Summary

Similarly to other patches of mine, I'm trying to uniformize the checker interface so that dependency checkers don't emit diagnostics. The checker that made me most anxious so far was definitely RetainCount, because it is definitely impacted by backward compatibility concerns, and implements a checker hierarchy that is a lot different to other examples of similar size. Also, I don't have authority, nor expertise regarding ObjC related code, so I welcome any objection/discussion!

Diff Detail

Event Timeline

Szelethus created this revision.Apr 14 2020, 4:56 AM
martong added inline comments.Apr 15 2020, 7:05 AM
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
1499

So, this is an intermediate patch before we reach that? That is the reason why we must use unique_ptrs and lazily init them?

1507

Perhaps #undef INIT_BUGTYPE would be useful here.

Szelethus marked an inline comment as done.Apr 15 2020, 7:47 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
1499

I lack the knowledge required to do that myself, unfortunately, and I'm no client of this checker, I'm not even sure what I would want to see it do.

We need unique pointers because BugType has no default constructor, and we receive the checker name after adding the checker object to CheckerManager (this happens in registerRetainCountBase). Other checkers use unique pointers for this type as well.

The lazy init, as opposed to simple initializing happens so that the checker name associated with the bug report doesn't depend on the registration order of RetainCount and OSObjectRetainCount (I guess I could've explained this in the code as well, huh). Right now, it only depends on which checker is enabled, which is bad enough, but addressing this TODO will fix that in an instant.

Ping, @NoQ @vsavchenko I'm confident with the previous patch, but I'd be glad if one of you could take a look before I land this.

martong requested changes to this revision.May 21 2020, 4:48 AM

Found a small mistake that should be corrected, then will be good to me.

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
15

Why adding this extra header? Don't blame clangd :D

clang/test/Analysis/test-separate-retaincount.cpp
5

This is mixed up with line 1, this line should be no-os-object.

This revision now requires changes to proceed.May 21 2020, 4:48 AM
Szelethus updated this revision to Diff 265729.May 22 2020, 6:24 AM
Szelethus marked 5 inline comments as done.

Fixes according to reviewer comments!

This revision is now accepted and ready to land.May 25 2020, 6:16 AM
This revision was automatically updated to reflect the committed changes.