-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[cxx-interop] Adding swift_name attributes to virtual methods overrides #83067
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
[cxx-interop] Adding swift_name attributes to virtual methods overrides #83067
Conversation
@swift-ci please smoke test |
It could be a follow-up PR but I think we should emit a warning when:
This sounds a bit harsh. Is it possible to do something else, e.g., making it unavailable? |
@Xazax-hun you think we should emit a warning in those 2 situations even if the method is not being used? This is feasible, but I'm wondering if it would generate too many warnings
Sorry, my description wasn't very explicit, but that's exactly what I'm doing: marking the method unavailable and, if the method is called, then the program won't compile because the method is unavailable |
lib/ClangImporter/ImportDecl.cpp
Outdated
Impl.markUnavailable(result, unavailabilityMsg); | ||
|
||
} else { | ||
// If the swift_name attribute renames the function to the same as |
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 we want to do the opposite and only import the derived method in this case.
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.
Sure! In the case where the override has the same swift_name
, we synthesize a new thunk for the method.
However, when the base method has a swift_name
but the override doesn't, we still skip the generation of a thunk and clone the one from the base method. I improved the comment explaining why this is correct.
f72f574
to
924de3e
Compare
@swift-ci please smoke test |
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.
Mostly looks good to me, I have some nits and questions inline.
8ebc8e1
to
5d87aec
Compare
5d87aec
to
585ca5e
Compare
@swift-ci please smoke test |
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.
LGTM, thanks!
The design for overriding virtual methods that may be renamed with a
swift_name
attribute is as follows:swift_name
attributeswift_name
attribute or have a different oneswift_name
attribute on a virtual method override is going to be ignored. We emit a warning and the method will be marked unavailableFollow up to #82485
rdar://155032235