-
Notifications
You must be signed in to change notification settings - Fork 46
[JExtract] Import any C compatible closures #253
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
919e3fd
to
83ffb88
Compare
printer.print( | ||
""" | ||
public interface \(functionType.name) extends \(cdeclDescritor).Function { | ||
default MemorySegment toUpcallStub(Arena arena) { |
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.
Maybe it's better to make a static func outside the interface, so it won't leak toUpcallStub
method?
private static func $toUpcallStub(\(functionType.name) fi, Arena arena) { ... }
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.
Agreed, that'll be nicer. In order keep the interface without default impls so users aren't confused what those funcs are about.
Most importantly we want only one non-default method, but it'll be nice to have those not have any defaulted ones either
/// return Linker.nativeLinker().upcallStub(HANDLE.bindTo(fi), DESC, arena); | ||
/// } | ||
/// } | ||
/// ``` |
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.
That's pretty good structure, I like it 👍
_ cType: CType | ||
) { | ||
guard case .pointer(.function(let cResultType, let cParameterTypes, variadic: false)) = cType else { | ||
preconditionFailure("must be a C function pointer type") |
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.
Include the name and cType in the crash message?
) { printer in | ||
printer.print( | ||
""" | ||
public interface Function { |
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 interface Function { | |
@FunctionalInterface | |
public interface Function { |
This isn't "required" in order for an interface to be a functional interface, but it's a good pattern.
What this does is that if we accidentally added one more non-default method to the interface then the java compiler would then error about it. That's important because we want callers to use the () -> {} lambda syntax rather than spell out the explicit types
printer.print( | ||
""" | ||
public interface \(functionType.name) extends \(cdeclDescritor).Function { | ||
default MemorySegment toUpcallStub(Arena arena) { |
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.
Agreed, that'll be nicer. In order keep the interface without default impls so users aren't confused what those funcs are about.
Most importantly we want only one non-default method, but it'll be nice to have those not have any defaulted ones either
) | ||
} else { | ||
// Otherwise, the lambda must be wrapped with the lowered function instance. | ||
assertionFailure("should be unreachable at this point") |
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.
If always unreachable, should we make this fatal error and remove the code below it? It's not like we'll be testing if that code is correct after all right?
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.
I'd prefer to keep the code below for now. It illustrates how things would work for non-C-compat closures, and it's also something I plan to implement pretty soon.
|
||
/// Function interfaces (i.e. lambda types) required for the Java method. |
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.
/// Function interfaces (i.e. lambda types) required for the Java method. | |
/// Function interfaces (i.e. functional interface types) required for the Java method. |
Nitpick but there's no real lambda types in java, just functional interface types which happen to get implemented with a lambda expression
var parameters: [TranslatedParameter] | ||
var result: TranslatedResult | ||
|
||
var isTrivial: Bool { |
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 trivial the right word? or are those c-convention compatible or something like that?
) | ||
} | ||
|
||
/// Translate Swift closure type to Java function interface. |
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.
/// Translate Swift closure type to Java function interface. | |
/// Translate Swift closure type to Java functional interface. |
minor wording nitpick
Sorry about the conflicts 🙏 No more big "move everything" incoming now anymore I think after the big command refactor. |
4c8ddf2
to
c40cf47
Compare
(holding off merging until #249 is merged) |
#249 is merged so this needs a rebase and can be merged at will! 👍 |
Generalized closure parameter import, part 1. Start with C-compatible closures which don't need any conversions.
c40cf47
to
7860cb8
Compare
Generalized closure parameter import, part 1.
Start with C-compatible closures which don't need any conversions.
For this Swift API:
The binding descriptor class has additional nested classes that describes the lowered functional interface and the method handle for each closure parameter.
Additional nested class containing the "wrapper" functional interfaces:
The wrapper method converts the "wrapper" lambda to the lowered lambda, and allocate the upcall stub by calling
.toUpcallStub(arena$)