[cxx-interop] Support importing static factories as initializers #79288
Conversation
Xazax-hun
requested review from
zoecarver,
hyp,
egorzhdan,
beccadax and
ian-twilightcoder
as code owners
| int value = 0; | ||
|
|
||
| __attribute__((swift_name("init(_:)"))) | ||
| __attribute__((swift_attr("returns_retained"))) |
There was a problem hiding this comment.
Would using returns_unretained be reasonable here in some cases?
There was a problem hiding this comment.
Hmm. Do you mean we should assume returns_retained? Or the fact that a user could pass in 0? I change the test to not pass in the initial refcount.
There was a problem hiding this comment.
Not necessarily, I just wanted to check if we should test the other possible annotation, returns_unretained
There was a problem hiding this comment.
Ah, I see. It feels weird for a ctor to do returns_unretained. I think we should warn on that. I can do that in a follow-up PR.
There was a problem hiding this comment.
Should we allow importing static factories as both returns_retained and returns_unretained for now? Once we have support for importing actual C++ constructors as Swift initializers then we can maybe think about a default behaviour for constructors and static factory methods. Seems like the default should be returns_retained but I am not aware if people are currently using returns_unretained for static factories.
There was a problem hiding this comment.
I will add a test for the returns_unretained case. I think it might be a good idea to add diagnostics in a separate PR in this case as those can be reverted independently of the feature itself if we encounter an idiom we did not think about.
Xazax-hun
force-pushed
the
gaborh/import-static-factory-as-initializer
branch
3 times, most recently
from
c893fac to
28ac70b
Compare
| @@ -3907,6 +3907,11 @@ namespace { | |||
| // FIXME: Poor location info. | |||
| auto nameLoc = Impl.importSourceLoc(decl->getLocation()); | |||
|
|
|||
| if (auto method = dyn_cast<clang::CXXMethodDecl>(decl); | |||
There was a problem hiding this comment.
@Xazax-hun why did you choose this location in the importFunctionDecl function to implement this logic?
There was a problem hiding this comment.
This is the importFunctionDecl function that has most of the function importing logic. We usually calculate the Swift Name for an imported declaration relatively early on. So this logic belongs here, and relatively early in this function (but after we have the SwiftName). There is another call site of importGlobalAsInitializer in the same function to handle the non-C++ method case, which is a good indication that this is the right place.
The easiest way to find where to put some code is to step through the import process of a simple example in the debugger and see what is the earliest point where you have all the information you need and check where would the code logically belong after that point.
There was a problem hiding this comment.
Another consideration is that we want to do this before we did any actual importingn (creating Swift AST) because we want to avoid importing the same function twice in two different ways.
| @@ -0,0 +1,36 @@ | |||
| #pragma once | |||
|
|
|||
| struct __attribute__((swift_attr("import_as_ref"))) | |||
There was a problem hiding this comment.
Should we use "import_refernece" or "import_as_ref" for these tests?
There was a problem hiding this comment.
Good point! We actually should use import_reference.
This PR adds a feature to import static factory functions returning foreign reference types to be imported as initializers using the SWIFT_NAME annotation.
Xazax-hun
force-pushed
the
gaborh/import-static-factory-as-initializer
branch
from
28ac70b to
2f46d55
Compare
|
@swift-ci please smoke test |
This PR adds a feature to import static factory functions returning foreign reference types to be imported as initializers using the SWIFT_NAME annotation.