Page MenuHomePhabricator

[Analyzer][WebKit] NoUncountedMembersChecker
Needs ReviewPublic

Authored by jkorous on Mar 31 2020, 2:43 PM.

Details

Reviewers
NoQ

Diff Detail

Event Timeline

jkorous created this revision.Mar 31 2020, 2:43 PM
jkorous updated this revision to Diff 264796.Mon, May 18, 10:31 PM

Updated to reflect feedback from review of the parent patch.

NoQ added inline comments.Thu, May 21, 8:26 AM
clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
106–107

Did you intentionally make a stronger check than the usual SourceManager::isInSystemHeader()? If so i'm curious why - like, what's so special about this checker that requires this? Should we fix all of our other checkers? It must be something something module maps. I don't know much about those.

110

This looks unused.

111

As far as i'm aware, in C++ every RecordDecl is a CXXRecordDecl.

112

What's the motivation for skipping those?

129–134

Can we elaborate a bit on why it's bad? Even something like ... which is error-prone would be great.

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp
10

I found members:: fairly unexpected. Maybe simply Class::Member? The surrounding namespace is obvious from the location of the warning and doesn't seem to be worth dumping.

jkorous marked 9 inline comments as done.Fri, May 22, 5:59 PM
jkorous added inline comments.
clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
106–107

No, there was no intention. I'll just use SourceManager::isInSystemHeader(). Thanks for pointing this out!

110

Indeed! Thanks

112

This checker enforces that there are no raw-pointers/references to uncounted types as member variable but since that's actually how WebKit smartpointers are implemented we need to skip those.
I'll add a comment.

129–134

Ok. Shall I add it to the checkers.rst or here?

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp
10

Ok.

jkorous updated this revision to Diff 265828.Fri, May 22, 6:00 PM
jkorous marked 3 inline comments as done.
  • rebased
  • addressed some of the comments