Skip to content

[cxx-interop] Prevent usage in Swift of C++ copy constructor with default args #79325

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 4, 2025
Merged

[cxx-interop] Prevent usage in Swift of C++ copy constructor with default args #79325

merged 1 commit into from
Mar 4, 2025

Conversation

susmonteiro
Copy link
Contributor

@susmonteiro susmonteiro commented Feb 12, 2025 •
edited
Loading

While support for C++ copy constructors with default args in Swift isn't added, we make these C++ types non-copyable.

rdar://142414553

@susmonteiro susmonteiro force-pushed the susmonteiro/copy-constructor-default-args branch from 35e7db5 to 47e556e Compare February 12, 2025 18:20
@susmonteiro susmonteiro marked this pull request as ready for review February 12, 2025 18:36
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro added the c++ interop Feature: Interoperability with C++ label Feb 12, 2025
Xazax-hun
Xazax-hun reviewed Feb 13, 2025
View reviewed changes
egorzhdan
egorzhdan reviewed Feb 13, 2025
View reviewed changes
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.

Looks great! @Xazax-hun raised a great question, once that's addressed, this PR should be good to go.

As a side note, we'll probably need a similar change for move constructors (see hasMoveTypeOperations), but that can be a separate patch.

@susmonteiro susmonteiro force-pushed the susmonteiro/copy-constructor-default-args branch from 47e556e to dacb9a6 Compare February 13, 2025 16:07
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

Xazax-hun
Xazax-hun approved these changes Feb 13, 2025
View reviewed changes
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.

LG!

egorzhdan
egorzhdan approved these changes Feb 13, 2025
View reviewed changes
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.

LGTM, thanks!

@susmonteiro susmonteiro force-pushed the susmonteiro/copy-constructor-default-args branch from dacb9a6 to f27af74 Compare February 14, 2025 17:37
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro force-pushed the susmonteiro/copy-constructor-default-args branch from f27af74 to b130614 Compare February 18, 2025 10:01
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@susmonteiro susmonteiro force-pushed the susmonteiro/copy-constructor-default-args branch from b130614 to 51b7ac3 Compare February 18, 2025 13:48
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

@rjmccall
Copy link
Contributor

IIRC, this is a fairly common pattern in some codebases, so while it's reasonable in the short term, we really do need to emit a thunk that we can call from Swift sooner rather than later.

@susmonteiro susmonteiro mentioned this pull request Feb 21, 2025
Merged
@susmonteiro susmonteiro force-pushed the susmonteiro/copy-constructor-default-args branch from 51b7ac3 to bc6573e Compare March 3, 2025 13:22
@susmonteiro susmonteiro requested a review from eeckstein as a code owner March 3, 2025 13:22
@susmonteiro
Copy link
Contributor Author

@swift-ci please smoke test

nate-chandler
nate-chandler approved these changes Mar 3, 2025
View reviewed changes
Copy link
Contributor

@nate-chandler nate-chandler left a comment

Choose a reason for hiding this comment

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

lgtm

@susmonteiro
Copy link
Contributor Author

@swift-ci Please smoke test macOS platform

@susmonteiro susmonteiro merged commit 593f320 into main Mar 4, 2025
3 checks passed
@susmonteiro susmonteiro deleted the susmonteiro/copy-constructor-default-args branch March 4, 2025 15:40
Xazax-hun
Xazax-hun reviewed Mar 5, 2025
View reviewed changes
lib/IRGen/GenStruct.cpp
@@ -541,12 +541,20 @@ namespace {
FixedTypeInfo, ClangFieldInfo> {
const clang::RecordDecl *ClangDecl;

bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is there a way to deduplicate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open a similar pr for move constructors soon. I'll see if I can fix this there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xazax-hun Xazax-hun Xazax-hun approved these changes

@egorzhdan egorzhdan egorzhdan approved these changes

@nate-chandler nate-chandler nate-chandler approved these changes

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@hyp hyp Awaiting requested review from hyp

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

@ian-twilightcoder ian-twilightcoder Awaiting requested review from ian-twilightcoder ian-twilightcoder is a code owner

@j-hui j-hui Awaiting requested review from j-hui j-hui is a code owner

@fahadnayyar fahadnayyar Awaiting requested review from fahadnayyar fahadnayyar is a code owner

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@susmonteiro @rjmccall @Xazax-hun @egorzhdan @nate-chandler