Skip to content

[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

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

susmonteiro
Copy link
Contributor

rdar://140994231

@susmonteiro susmonteiro added the c++ interop Feature: Interoperability with C++ label Jan 28, 2025
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@Xazax-hun Xazax-hun left a 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.

@susmonteiro susmonteiro force-pushed the susmonteiro/operators-unnamed-param branch from 9a62fdb to a4c512a Compare January 28, 2025 13:37
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@egorzhdan egorzhdan left a 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!

Comment on lines 2217 to 2221
if (param->getParameterName().empty()) {
param->setName(ctx.getIdentifier("other"));
}
Copy link
Contributor

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.

@susmonteiro susmonteiro force-pushed the susmonteiro/operators-unnamed-param branch from a4c512a to b7c5a47 Compare January 28, 2025 16:26
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro merged commit cf8a5bd into main Jan 29, 2025
3 checks passed
@susmonteiro susmonteiro deleted the susmonteiro/operators-unnamed-param branch January 29, 2025 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants