Skip to content

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

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented Oct 23, 2023

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html labels Oct 23, 2023
@github-actions
Copy link

github-actions bot commented Oct 23, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@rniwa rniwa force-pushed the fix-webkit-ref-deref-on-diff-classes branch from d823635 to 1832737 Compare October 24, 2023 00:20
@haoNoQ
Copy link
Collaborator

haoNoQ commented Nov 9, 2023

LGTM, thanks!! I have a couple nitpicks but I don't think they should block your progress.

@@ -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");
Copy link
Collaborator

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!

@haoNoQ
Copy link
Collaborator

haoNoQ commented Nov 9, 2023

(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!)

@rniwa rniwa force-pushed the fix-webkit-ref-deref-on-diff-classes branch from 1832737 to bf169f3 Compare November 9, 2023 23:44
@rniwa
Copy link
Contributor Author

rniwa commented Nov 10, 2023

Yes, it would be great if you can merge the change now that I've addressed your nits.

@haoNoQ haoNoQ merged commit 09273d4 into llvm:main Nov 10, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
haoNoQ pushed a commit to swiftlang/llvm-project that referenced this pull request Dec 16, 2023
…fined on different classes. (llvm#69985)

(cherry picked from commit 09273d4)
@rniwa rniwa deleted the fix-webkit-ref-deref-on-diff-classes branch March 6, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants