[cxx-interop] allow shared ref retain function to return self #77837
Conversation
lhoward
requested review from
zoecarver,
hyp,
egorzhdan,
Xazax-hun,
beccadax and
ian-twilightcoder
as code owners
|
Note to self: need separate error message for invalid retain return value. |
lhoward
force-pushed
the
lhoward/retain-return-self
branch
from
c077f57 to
85ea760
Compare
lhoward
force-pushed
the
lhoward/retain-return-self
branch
2 times, most recently
from
d9903f1 to
46c2da4
Compare
|
Tests are now failing, investigating. |
lhoward
force-pushed
the
lhoward/retain-return-self
branch
from
46c2da4 to
8d56f5a
Compare
|
Tests passing again. |
|
LGTM |
|
Just double checking we don't need to do anything extra at the retain call site to ignore the return value. |
lib/ClangImporter/ImportDecl.cpp
Outdated
| auto resultInterfaceType = operationFn->getResultInterfaceType(); | ||
| if (!resultInterfaceType->isVoid()) { | ||
| if (operationKind == CustomRefCountingOperationKind::release || | ||
| resultInterfaceType->getAnyNominal() != paramDecl) |
There was a problem hiding this comment.
This check isn't correct: it's comparing the declaration result's nominal type (if there is one) against the parameter declaration itself (which has, e.g., the name of the parameter in it). Instead, you need to compare the result type against the parameter type. I also expect that we need to look through an optional type on the result type, in case the retain function handles null pointers in some manner. Specifically, I think it should be:
| resultInterfaceType->getAnyNominal() != paramDecl) | |
| !resultInterfaceType->lookThroughSingleOptionalType()->isEqual(paramType)) |
There was a problem hiding this comment.
I'm confused about why this PR worked without this change suggested above. Running tests in CI without the change because I would expect it to fail.
There was a problem hiding this comment.
Fix applied (shouldn't disrupt running builds to check previous commit).
| @@ -2534,24 +2534,23 @@ namespace { | |||
| void validateForeignReferenceType(const clang::CXXRecordDecl *decl, | |||
| ClassDecl *classDecl) { | |||
|
|
|||
| enum class RetainReleaseOperatonKind { | |||
| enum class RetainReleaseOperationKind { | |||
There was a problem hiding this comment.
Ah, thank you for the typo fix here!
|
@swift-ci please smoke test macOS |
Many existing C APIs for retaining references, including Apple's own, return the reference. Support this pattern, along with the existing void return signature, with when importing reference types from C++.
lhoward
force-pushed
the
lhoward/retain-return-self
branch
from
8d56f5a to
da23bcf
Compare
|
@swift-ci please smoke test |
|
What's the next release one might expect to see this in? |
|
Once it's merged to main, we can cherry-pick to the |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux |
|
@lhoward I'm going to merge now. Would you cherry-pick this to the release/6.1 branch? There are instructions at https://forums.swift.org/t/swift-6-1-release-process/75442#p-344524-pull-requests-for-release-branch-8 |
|
Retain functions that return self need to be manually annotated with |
Many existing C APIs for retaining references, including Apple's own, return the reference. Support this pattern, along with the existing
voidreturn signature, with when importing reference types from C++.More: https://forums.swift.org/t/swift-shared-reference-signature-error/76194/7