-
Notifications
You must be signed in to change notification settings - Fork 75
[jextract] Add async legacy mode #462
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
| /// Extract Swift `async` APIs as Java functions that return `Future`s. | ||
| /// | ||
| /// This mode is useful for platforms that do not have `CompletableFuture` support, such as | ||
| /// Android 23 and below. |
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.
Please have this framed about java versions first and foremost, and about Android versions secondary. CompletableFuture is JDK8 which is pretty ancient
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.
Hm, its not really about JDK versions either. We generate code that uses JDK 17 language features. This is just a result of Android adopting the Java library features at different times and in later API levels
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.
Ignore android exists for a moment; this is absolutely a JDK question. Android isn't JDK so we mention the Android version, for anyone else, this is specifically about JDK versions.
SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftLegacyFuture.java
Outdated
Show resolved
Hide resolved
SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftLegacyFuture.java
Outdated
Show resolved
Hide resolved
| void asyncString() throws Exception { | ||
| Future<String> future = MySwiftLibrary.asyncString("hey"); | ||
| assertEquals("hey", future.get()); | ||
| } |
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.
Why these changes if we did not change the generation mode in this sample?
We should find a way to test both modes, we don't want to have zero coverage of either runtime functionality
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.
Just makes it easier to switch between the two modes when testing. These tests should work no matter what Future type is used.
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.
Yeah we're lucky enough that these modes are source compatible on this basic level. We should make the verification process make use of that though -- in the long term we'd need source directories for specific modes, but for this specific PR how about we add another step that sets an env var, that env var then the ci-validate script picks up and changes the mode in the swift-java config.
We're growing more "modes" and it's probably time we start actually testing them, even if it's a rarely used mode like this.
Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+JavaTranslation.swift
Outdated
Show resolved
Hide resolved
| if (isDone()) { | ||
| runCallbacks(); | ||
| } | ||
| } |
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 is racy but "kinda works out" I guess... It is racy because a complete could totally happen as we: started runCallbacks here, complete triggers runCallbacks -- and we then have two concurrent runCallbacks running (bad). It kinda works out because the queue is ConcurrentLinkedDeque so it won't yield us the same result twice...
So it's a bit crude but I guess it may be good enough for a first version here hm...
I'd like to add tests exercising this situation though please.
SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftLegacyFuture.java
Outdated
Show resolved
Hide resolved
SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftLegacyFuture.java
Outdated
Show resolved
Hide resolved
|
Hmm github won't let me suggest changes into this PR, so here's the alternative Future idea: please give it a go and add some tests as well for a few common cases |
ktoso
left a comment
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.
LGTM, please follow up with documentation
Adds a new
asyncFuncModecalledlegacyFuture. This is a mode that can be used on platforms that do not haveCompletableFutureavailable, such as Android 23. We just return a basicFuturetype, but under the hood we useSwiftLegacyFuture, which just has a basic "complete" functionality and ability to run a callback, that we need internally to convert the result.