-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Fix WebKit static analyzers to support ref and deref methods to be defined on different classes. #69985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
d823635
to
1832737
Compare
LGTM, thanks!! I have a couple nitpicks but I don't think they should block your progress. |
clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
Outdated
Show resolved
Hide resolved
@@ -77,14 +77,53 @@ class RefCntblBaseVirtualDtorChecker | |||
(AccSpec == AS_none && RD->isClass())) | |||
return false; | |||
|
|||
std::optional<const CXXRecordDecl*> RefCntblBaseRD = isRefCountable(Base); | |||
if (!RefCntblBaseRD || !(*RefCntblBaseRD)) | |||
auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this entire new section can be deduplicated with isRefCountable()
in the other file. They look so similar!
(You still need me to merge right? You can ask me to merge, or you can address nitpicks and then ask me to merge, or you can go through https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and merge yourself!) |
…fined on different classes.
1832737
to
bf169f3
Compare
Yes, it would be great if you can merge the change now that I've addressed your nits. |
…fined on different classes. (llvm#69985)
…fined on different classes. (llvm#69985) (cherry picked from commit 09273d4)
No description provided.