-
Notifications
You must be signed in to change notification settings - Fork 79
jextract/jni: Add initial Foundation.Data support #557
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
This reverts commit 84292bf.
965a925 to
e821819
Compare
e821819 to
118743f
Compare
|
Very nice! Could we add the same mechanism for |
|
Yep we can do that, following up |
| // array directly without this extra copy. For now, just map. | ||
| self = jniArray.map { Element(fromJNI: $0, in: environment) } | ||
| self = result as! Self | ||
| } else if Element.self == Int8.self { |
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.
Could not nicely pull this off in a single branch... doing some small local func was even more annoying tbh -- since we need the concrete Int8.jniGetArrayRegion
This also improves the Array.getJNIValue(in:) when the array is of bytes, addressing a long standing TODO.
I also worked on optimizing the
toByteArraysome more and now we're able to get such performance when doing the copying (by avoiding iterating and mapping data into jbytes):These work because uint8 and jbyte should be really have the same layout AFAIK so we don't need to iterate, but just rebind the memory and use the JNI funcs to initialize the java array. We could do even better by initializing into an existing array, but I didn't do that yet.
I did use the help of Claude (with lots of hand holding) to get some of the boilerplate done here, and especially make sure the FFM and JNI modes are roughly equivalent. I've read and vetted all the code and tests, and most documentation was written by myself.