Skip to content

[cxx-interop] import const reference parameters as __shared T #40350

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

Closed
wants to merge 1 commit into from

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Dec 1, 2021

This change allows Swift code to pass immutable values to const references in C++.
Note that const reference parameters in function template instantiations are not yet
supported in this patch, as even non-const refs are mapped as pointers instead of 'inout' parameters
in Swift. The support for reference parameters in function template instantions will be
added in a follow-up patch.

This change allows Swift code to pass immutable values to const references in C++.
Note that const reference parameters in function template instantiations are not yet
supported in this patch, as even non-const refs are mapped as pointers instead of 'inout' parameters
in Swift. The support for reference parameters in function template instantions will be
added in a follow-up patch.
@hyp hyp added the c++ interop Feature: Interoperability with C++ label Dec 1, 2021
@hyp
Copy link
Contributor Author

hyp commented Dec 1, 2021

@swift-ci please test

@@ -1926,7 +1926,8 @@ ParameterList *ClangImporter::Implementation::importFunctionParameterList(
} else {
if (auto refType = dyn_cast<clang::ReferenceType>(paramTy)) {
paramTy = refType->getPointeeType();
isInOut = true;
paramSpecifier = paramTy.isConstQualified() ? ParamSpecifier::Shared
Copy link
Contributor

Choose a reason for hiding this comment

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

"__shared" doesn't mean "shared value" is really means "non owning" (as Andrew helpfully reminded me). No matter what, It's still possible that copies will be made, since we don't support proper borrowing yet. But I'm told that's soon to come. (I think that's the same case for inout if the underlying object is constructed in Swift, so it's a bit of a mute point, though.)

Anyway, we shouldn't use "__shared" here. Let's just use the default parameter specifier and update it whenever we have something better.

CC @atrick

@@ -1289,6 +1289,17 @@ static bool isClangTypeMoreIndirectThanSubstType(TypeConverter &TC,

return true;
}

// Pass C++ const reference types indirectly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this comment to explain why this logic is necessary? I.e., how we don't have a better way to express that something's an immutable borrow/reference in Swift right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess another way to say it: the Swift AST has no way to represent const-ref until this point. So, we can't say const-ref in the importer, that's why this logic is here and not in the importer.

// Pass C++ const reference types indirectly.
if (clangTy->isReferenceType() &&
clangTy->getPointeeType().isConstQualified()) {
// FIXME: Template substitution references are still mapped as
Copy link
Contributor

Choose a reason for hiding this comment

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

What case is this covering? T&?

@@ -16,3 +16,19 @@ void setConstStaticIntRvalueRef(const int &&i) { staticInt = i; }

auto getFuncRef() -> int (&)() { return getStaticInt; }
auto getFuncRvalueRef() -> int (&&)() { return getStaticInt; }

void takeConstPODStructRef(const PODStruct &value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these can probably all go in the header (makes it easier to find in the future). The source file is just for the statics.

staticInt = value->x;
}

void takeConstPODStructPointerConstRvalueRef(PODStruct * _Nonnull const &&value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be importing these.

But also, this is a real anti-pattern. Clang should probably complain. And... should we ever import these?

@zoecarver
Copy link
Contributor

Closing in favor of #41611.

@zoecarver zoecarver closed this Mar 2, 2022
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.

2 participants