-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[cxx-interop] Fix crash when operator doesn't name parameter in header #78974
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
@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.
Looks good to me! Some nits inline but I am also happy leaving the PR as is.
9a62fdb
to
a4c512a
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!
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.
Just one comment, but otherwise this looks great!
if (param->getParameterName().empty()) { | ||
param->setName(ctx.getIdentifier("other")); | ||
} |
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.
Since this is changing the name of the parameter of the backing C++ function, can we clone the parameter using ParamDecl::clone
, and then change the name of the cloned parameter? I think this would be a good thing to do even if the name doesn't need to be changed, i.e. when if (param->getParameterName().empty())
doesn't trigger.
a4c512a
to
b7c5a47
Compare
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.
Awesome, thanks!
@swift-ci please smoke test |
1 similar comment
@swift-ci please smoke test |
rdar://140994231