[cxx-interop] Allow retain/release operations to be methods #84343
Conversation
egorzhdan
requested review from
zoecarver,
Xazax-hun,
j-hui,
fahadnayyar,
susmonteiro,
hnrklssn,
rjmccall,
beccadax and
ian-twilightcoder
as code owners
|
@swift-ci please test |
| static DerivedRefCountedBox *createDerived(int value, int secondValue) { | ||
| return new DerivedRefCountedBox(value, secondValue); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Do you think you might be kind enough to also add some templated ref counted base types along the lines of cntrump/MixingLanguagesInAnXcodeProject@main...adetaylor:MixingLanguagesInAnXcodeProject:demonstrate-refcount-probs? Just to make sure this works (and continues to work) for cases like WebKit's? I suspect that the pattern is fairly common since it's a good way to do reference counting without the overhead of virtual functions.
There was a problem hiding this comment.
Good point, let's test that scenario explicitly.
| } | ||
|
|
||
| void doRelease() const { refCount--; } | ||
| } SWIFT_SHARED_REFERENCE(.doRetainInBase, .doRelease); |
There was a problem hiding this comment.
What happens when you have a base class FRT with virtual retain and release methods, and the derived class overrides its retain and release functions? And what about for pure virtual methods?
There was a problem hiding this comment.
This is a great point! In case this does not work as intended and not easy to fix, I believe we could just reject virtual refcounting functions for now and fix these in a follow-up PR.
There was a problem hiding this comment.
That works! We should have a test for it indeed, I just added one.
egorzhdan
force-pushed
the
egorzhdan/retain-release-methods
branch
from
a54fc65 to
7c9856d
Compare
lib/ClangImporter/ImportDecl.cpp
Outdated
| @@ -2801,17 +2801,24 @@ namespace { | |||
| if (!operationFn) | |||
| return RetainReleaseOperationKind::notAfunction; | |||
|
|
|||
| if (operationFn->getParameters()->size() != 1) | |||
| if (operationFn->getParameters()->size() + operationFn->isInstanceMember() != 1) | |||
There was a problem hiding this comment.
Does this mean we will allow static member functions? E.g.:
struct FRT {
static void retain(FRT*) { ... }
};
I think we should either reject this or have a test.
There was a problem hiding this comment.
Added logic to reject static functions for now. I think we'll need to figure out what the design is for static functions and inheritance before we allow this.
|
|
||
| RefCountedBox(int value) : value(value) {} | ||
|
|
||
| static RefCountedBox *create(int value) { return new RefCountedBox(value); } |
There was a problem hiding this comment.
Nit: we now import ctors of foreign reference types so we could potentially simplify some of these examples.
There was a problem hiding this comment.
Ah right, good point, removed these static factories.
| // MARK: Retain in a base type, release in templated derived | ||
|
|
||
| template <typename T> | ||
| struct TemplatedDerivedHasRelease : BaseHasRetain { |
There was a problem hiding this comment.
I think we might want to have a test following the curiously recurring template pattern where the base itself is templated. Similar to RefCountedBase in LLVM. We usually need the CRTP for the release operation.
There was a problem hiding this comment.
Added a test for that scenario.
Xazax-hun
dismissed
their stale review
I actually want the override case discussed/tested before this is merged.
egorzhdan
force-pushed
the
egorzhdan/retain-release-methods
branch
from
7c9856d to
654ddf2
Compare
Some foreign reference types such as IUnknown define retain/release operations as methods of the type. Previously Swift only supported retain/release operations as standalone functions. The syntax for member functions would be `SWIFT_SHARED_REFERENCE(.doRetain, .doRelease)`. rdar://160696723
egorzhdan
force-pushed
the
egorzhdan/retain-release-methods
branch
from
654ddf2 to
9c5de6f
Compare
|
@swift-ci please test |
Some foreign reference types such as IUnknown define retain/release operations as methods of the type.
Previously Swift only supported retain/release operations as standalone functions.
The syntax for member functions would be
SWIFT_SHARED_REFERENCE(.doRetain, .doRelease).rdar://160696723