Skip to content

[cxx-interop] Remove redundant condition for invoking move ctor #77893

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
Dec 6, 2024

Conversation

Xazax-hun
Copy link
Contributor

@Xazax-hun Xazax-hun commented Dec 2, 2024

There is no reason to require a body for move/copy ctors to use them.

@Xazax-hun Xazax-hun added the c++ interop Feature: Interoperability with C++ label Dec 2, 2024
@Xazax-hun Xazax-hun requested a review from rjmccall as a code owner December 2, 2024 11:21
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@Xazax-hun Xazax-hun requested a review from egorzhdan December 2, 2024 11:27
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This is fine to merge, but I wonder if we should just delete the line that follows as it is already commented out. Or was the radar related to the deleted ctor?

@Xazax-hun
Copy link
Contributor Author

The rdar was about uncommenting the commented out line but did not provide any context what is blocking us from doing that.

@rjmccall
Copy link
Contributor

rjmccall commented Dec 2, 2024

Okay. Note that you do still have that line in the code about move constructors, and it's weird to be using different rules for them.

I was hoping the explanatory comment was going to be something like "There's a bug in how we interact with the fact that C++ does blah blah blah with inline copy constructors, so don't require a body for now". If we really don't know why we had that condition in the first place, I tend to agree with Saleem that we should just remove it (in both places). Abstractly, I certainly don't see any reason why we would only look for constructors defined inline in the class, which seems to be what this condition is doing, so if there's a reason we do need to do that, it deserves a real comment regardless.

@Xazax-hun
Copy link
Contributor Author

it deserves a real comment regardless

I agree with everything you said. Unfortunately, I looked at the commit that introduced this code and the rdar that was linked but none of them provided the context why the two methods are not symmetric.

I will give removing this call in both places a try and see if anything breaks.

There is no reason to require a body for move/copy ctors to use them.
@Xazax-hun
Copy link
Contributor Author

@swift-ci please test

@Xazax-hun Xazax-hun changed the title [cxx-interop] Remove stale reference to rdar [cxx-interop] Remove redundant condition for invoking move ctor Dec 3, 2024
@Xazax-hun
Copy link
Contributor Author

@swift-ci please test

@Xazax-hun Xazax-hun merged commit 7284676 into main Dec 6, 2024
5 checks passed
@Xazax-hun Xazax-hun deleted the gaborh/remove-rdar branch December 6, 2024 05:42
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.

4 participants