-
Notifications
You must be signed in to change notification settings - Fork 46
Separate SwiftKit into SwiftKitCore and SwiftKitFFM #300
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
I think that's the right call, but let's avoid getting the method via reflection and instead just give the cleanup something to run, rather that reflection trick and it'll be fine 👍 |
BuildLogic/src/main/kotlin/build-logic.java-common-conventions.gradle.kts
Outdated
Show resolved
Hide resolved
Samples/SwiftAndJavaJarSampleLib/src/jmh/java/org/swift/swiftkit/JavaToSwiftBenchmark.java
Outdated
Show resolved
Hide resolved
SwiftKitFFM/src/main/java/org/swift/swiftkitffm/AllocatingSwiftArena.java
Outdated
Show resolved
Hide resolved
SwiftKitFFM/src/main/java/org/swift/swiftkitffm/AllocatingSwiftArena.java
Outdated
Show resolved
Hide resolved
SwiftKitCore/src/main/java/org/swift/swiftkitcore/JNISwiftInstanceCleanup.java
Outdated
Show resolved
Hide resolved
SwiftKitFFM/src/main/java/org/swift/swiftkitffm/FFMSwiftInstanceCleanup.java
Outdated
Show resolved
Hide resolved
SwiftKitCore/src/main/java/org/swift/swiftkitcore/SwiftLibraries.java
Outdated
Show resolved
Hide resolved
- name: Gradle :SwiftKitFFM:build | ||
run: ./gradlew :SwiftKitFFM:build -x test | ||
- name: Gradle :SwiftKitFFM:check | ||
run: ./gradlew :SwiftKitFFM:check --info |
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 that's nice actually to keep them separate rather than one big :check
@@ -27,9 +27,10 @@ fi | |||
|
|||
# check if we can compile a plain Example file that uses the generated Java bindings that should be in the generated jar | |||
# The classpath MUST end with a * if it contains jar files, and must not if it directly contains class files. | |||
SWIFTKIT_CLASSPATH="$(pwd)/../../SwiftKit/build/libs/*" | |||
SWIFTKIT_CORE_CLASSPATH="$(pwd)/../../SwiftKitCore/build/libs/*" | |||
SWIFTKIT_FFM_CLASSPATH="$(pwd)/../../SwiftKitFFM/build/libs/*" |
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, sorry you had to go through figuring that out, but now you know hah :)
@@ -148,7 +147,8 @@ tasks.clean { | |||
} | |||
|
|||
dependencies { | |||
implementation(project(':SwiftKit')) | |||
implementation(project(':SwiftKitCore')) | |||
implementation(project(':SwiftKitFFM')) |
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.
Nitpick, no need to fix up right now -- since FFM depends on Core we could just list the FFM dependency here, it'd resolve properly I think even with project dependencies
|
||
@Override | ||
public void run() { | ||
markAsDestroyed.run(); |
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.
We can do this in a follow up but we should only run the destroy if the mark permits us to do so; we can fix that later though
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.
Looking good, let's get that in :)
The goal of this PR is to separate SwiftKit into two modules:
SwiftKitCore
: A module that supports non-FFM memory management and uses JNI for any downcalls.SwiftKitFFM
: A module that uses FFM for allocation of memory and downcalls.