-
Notifications
You must be signed in to change notification settings - Fork 46
[jextract] Rework translation #236
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
Conversation
e5111e9
to
5bc7668
Compare
a01680f
to
fcb71fe
Compare
SwiftKit/src/main/java/org/swift/swiftkit/SwiftInstanceCleanup.java
Outdated
Show resolved
Hide resolved
SwiftKit/src/main/java/org/swift/swiftkit/SwiftInstanceCleanup.java
Outdated
Show resolved
Hide resolved
SwiftKit/src/main/java/org/swift/swiftkit/SwiftValueWitnessTable.java
Outdated
Show resolved
Hide resolved
Sources/JExtractSwift/Swift2JavaTranslator+JavaThunkPrinting.swift
Outdated
Show resolved
Hide resolved
fcb71fe
to
a51ba9b
Compare
a51ba9b
to
64fbf46
Compare
70f9407
to
d597a64
Compare
5732e36
to
54b9299
Compare
@@ -26,7 +26,7 @@ public struct ForeignValueLayout: CustomStringConvertible, Equatable { | |||
|
|||
public init(inlineComment: String? = nil, javaConstant: String) { | |||
self.inlineComment = inlineComment | |||
self.value = javaConstant | |||
self.value = "SwiftValueLayout.\(javaConstant)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
// When the type is some custom type, e.g. another Swift struct that we imported, | ||
// we need to import its layout. We do this by calling $layout() on it. | ||
// we need to import its layout. We do this by calling $LAYOUT() on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this because openjdk seems to use uppercase for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is $layout()
method as an instance method. I added $LAYOUT()
as a static method because this is rendered as ClassName.$LAYOUT()
. Maybe we could just use $LAYOUT
static field, but I wasn't sure it's OK to make it public
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final ... LAYOUT
field is fine I think, it'd help differentiate the field and method. TBD if we need both, but we can decide this later. Uppercase like that does look a bit weird so I'd go for a constant which then looks "expected" being uppercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Updated. While I am here, I removed needsMemoryLayoutCall
from the type and added .$LAYOUT
in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks 👍
Sources/JExtractSwift/Swift2JavaTranslator+JavaBindingsPrinting.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all fine to me, thank you @rintaro.
Just few minor comments, one of which you already fixed, which can be done anytime in the future. LGTM, please merge at will 👍
FYI @madsodgaard -- with this merged, it'll be a good time for you to introduce the protocol to pick between ffm and jni modes 👍
} | ||
|
||
for (param, isLast) in cFunc.parameters.withIsLast { | ||
printer.print("/* \(param.name ?? "_"): */", .continue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor / doesn't have to be in this PR: I think we had an effectiveParamLabel somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually never be nil
. cFunc
is created from LoweredFunctionSignature
created by lowerFunctionSignature()
which actually gives a name to every parameter.
* Create an instance of {@code \(className)}. | ||
* | ||
* {@snippet lang=swift : | ||
* \(decl.signatureString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up:
Silly but I think now if a signature contains /**/
because we take the raw syntax with the trivia that would break rendering of the source file because the */ will end the comment.
We probably should sanitize that and add a test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added FIXME on ImportedFunc.signatureString
case .getter: "get\(decl.name.toCamelCase)" | ||
case .setter: "set\(decl.name.toCamelCase)" | ||
case .function: decl.name | ||
case .initializer: fatalError("unreachable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give this a better reason string why we crash?
self.needsMemoryLayoutCall = true | ||
// When the type is some custom type, e.g. another Swift struct that we imported, | ||
// we need to import its layout. We do this by referring $LAYOUT on it. | ||
self.value = "\(customType).$LAYOUT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for updating that to a property
returnTy = typeAnnotation.type | ||
} else { | ||
returnTy = "Swift.Void" | ||
func importAccessor(kind: SwiftAPIKind) throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
extension SyntaxProtocol { | ||
/// Look in the comment text prior to the node to find a mangled name | ||
/// identified by "// MANGLED NAME: ". | ||
var mangledNameFromComment: String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup we dont need this anymore, thank you
// } | ||
for decl in nominal.methods { | ||
decls.append(contentsOf: render(forFunc: decl)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, variables - thank you
* Utilize `SwiftFunctionSignature` and the cdecl-lowering facilities throughout the code base. * `SwiftFunctionSignature` is created from the `DeclSyntax` in `Swift2JavaVisitor`. * `LoweredFunctionSignature` describing `@cdecl` thunk, is created from `SwiftFunctionSignature`. * `TranslatedFunctionSignature` describing generated Java API, is created from `LoweredFunctionSignature`. (Swift2JavaTranslator+JavaTranslation.swift) * `ImportedFunc` is now basically just a wrapper of `TranslatedFunctionSignature` with the `name`. * Remove `ImportedVariable`, instead variables are described as `ImportedFunc` as accessors. * Support APIs returning imported type values. E.g. `func factory(arg: Int) -> MyClass` such methods require `SwiftArena` parameter passed-in. * Built-in lowerings (e.g. `toCString(String)` for `String` -> C string conversion) are now implemented in JavaKit. * Stop emitting `MyClass::apiName$descriptor()` method etc. as they were not used. * Use the `@cdecl` thunk name as the function descriptor class name, for simplicity. * Getter and setter accessors are now completely separate API. No more `HANDLE_GET` and `HANDLE_SET` etc. Instead descriptor class is split to `$get` or `$set` suffixed name.
c2d4cc5
to
637d800
Compare
SwiftFunctionSignature
and the cdecl-lowering facilities throughout the code base.SwiftFunctionSignature
is created from theDeclSyntax
inSwift2JavaVisitor
.LoweredFunctionSignature
describing@cdecl
thunk, is created fromSwiftFunctionSignature
.TranslatedFunctionSignature
describing generated Java API, is created fromLoweredFunctionSignature
. (Swift2JavaTranslator+JavaTranslation.swift)ImportedFunc
is now basically just a wrapper ofTranslatedFunctionSignature
with thename
.ImportedVariable
, instead variables are described asImportedFunc
as accessors.func factory(arg: Int) -> MyClass
such methods requireSwiftArena
passed-in.toCString(String)
forString
-> C string conversion) are now implemented in JavaKit.MyClass::apiName$descriptor()
method etc. as they were not used.@cdecl
thunk name as the function descriptor class name, for simplicity.HANDLE_GET
andHANDLE_SET
etc. Instead descriptor class is split to$get
or$set
suffixed name.Actual output of MyClass.java ▼