[cxx-interop] Default ownership convention as 3rd optional parameter of SWIFT_SHARED_REFERENCE annotation #79574
Conversation
fahadnayyar
force-pushed
the
cxx-frt-default-return-ownership
branch
from
667b5d9 to
28104c0
Compare
fahadnayyar
requested review from
zoecarver,
hyp,
egorzhdan,
Xazax-hun,
j-hui,
jckarter,
beccadax and
ian-twilightcoder
as code owners
|
@swift-ci please smoke test |
|
I wonder if something like |
| #define SWIFT_SHARED_REFERENCE_3ARGS(_retain, _release, _ownership) \ | ||
| __attribute__((swift_attr("import_reference"))) \ | ||
| __attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(retain:_retain)))) \ | ||
| __attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(release:_release)))) \ |
There was a problem hiding this comment.
Nit: formatting, the backslashes are not in the same column.
| __attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(retain:_retain)))) \ | ||
| __attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(release:_release)))) | ||
| #define _CXX_INTEROP_GET_MACRO(_1, _2, _3, NAME, ...) NAME | ||
| #define SWIFT_SHARED_REFERENCE(...) \ |
There was a problem hiding this comment.
Is there a way to define this as (_retain, _release, ...)? That would make it more readable for someone opening this header file.
| return ResultConvention::Owned; | ||
| if (tl.getLoweredType().isForeignReferenceType()) { | ||
| if (decl->hasAttrs()) { | ||
| for (const auto *attr : decl->getAttrs()) { |
There was a problem hiding this comment.
We have this pattern in our codebase so many times, I am wondering if we should have a helper function at this point.
Something like:
template<typename T>
std::optional<T> matchSwiftAttrs(const clang::Decl *d, ArrayRef<StringRef, T> matches) {
...
if (swiftAttr->getAttribute() == matches[i].first)
return matches[i].second;
...
}
So we could simplify all of the code snippets like this to:
if (auto match = matchSwiftAttrs(decl, {{"returns_unretained", ResultConvention::Unowned},
{"returns_retained", ResultConvention::Owned}})
return match;
There was a problem hiding this comment.
Addressed in PR-81093
| DefaultOwnershipSuppressUnannotatedAPIWarning::RefTypeDefaultRetained *v) { | ||
| }; | ||
|
|
||
| SWIFT_END_NULLABILITY_ANNOTATIONS |
There was a problem hiding this comment.
Nit: missing new line at the end of the file.
| SWIFT_BEGIN_NULLABILITY_ANNOTATIONS | ||
|
|
||
| namespace DefaultOwnershipConventionOnCXXForegnRefType { | ||
| struct __attribute__((swift_attr("import_reference"))) |
There was a problem hiding this comment.
I think for this change it would be important to add tests that are testing the bridging header directly.
There was a problem hiding this comment.
This should not be relevant now in PR-81093. But feel free to leave a comment on the new PR if you feel strongly about this.
…of SWIFT_SHARED_REFERENCE annotation rdar://145453509
fahadnayyar
force-pushed
the
cxx-frt-default-return-ownership
branch
from
28104c0 to
fc1d877
Compare
|
Closing this PR because I have started a new PR-81093 to address this problem. |
Since #76798 we started emitting warning for C++ APIs returning
SWIFT_SHARED_REFERENCEtypes when called from Swift. This adds some extra annotation burden on the C++ side.This patch is adding a way to to specify the return value ownership at the type level for C++ foreign reference types by extending
SWIFT_SHARED_REFERENCEwith an optional parameter takingretainedorunretainedlike:SWIFT_SHARED_REFERENCE (retainFunction, releaseFunction, retained)SWIFT_SHARED_REFERENCE (retainFunction, releaseFunction, unretained)The 3rd optional parameter expands to:
__attribute__((swift_attr("ownership:retained")))and
__attribute__((swift_attr("ownership:unretained")))repectively.
rdar://145453509