-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cxx-interop] Refactor copyability out of CxxRecordSemantics #84237
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] Refactor copyability out of CxxRecordSemantics #84237
Conversation
@swift-ci please smoke test |
lib/ClangImporter/ClangImporter.cpp
Outdated
} | ||
|
||
llvm_unreachable("Could not classify C++ type."); | ||
return CxxRecordSemanticsKind::Owned; |
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 think Owned
might not be the right default here. Owned
means that it cannot dangle. Maybe we need to introduce a placeholder here, like Value
?
}; | ||
|
||
enum class CxxRecordSemanticsKind { | ||
Trivial, |
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 am a bit conflicted what to do with Trivial
. Do you know what do we use this for? Because I think this might be more of a description of how the class is being copied/moved, passed around as an implementation detail rather than a semantic property.
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.
Yeah, I wonder if we can replace Trivial
with Value
to indicate a value type.
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.
So we would replace both Trivial
and Owned
with Value
?
The only place we use this distinction is in SwiftDeclSynthesizer::findExplicitDestroy
, when we emit a diagnostic for an illegal destroy operation. But I think I can work around that
382c581
to
3d7d38c
Compare
@swift-ci please smoke test |
@swift-ci please test windows |
3d7d38c
to
2640c71
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.
LG, thanks!
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.
Nice, thanks!
@swift-ci please test windows |
CxxRecordSemantics
determines how C++ APIs are imported to Swift, e.g., as owned, reference or iterator types. Since this is orthogonal to importing a type as copyable or move-only, we separateCxxRecordSemantics
fromCxxValueSemantics
.