Migrate to jni 0.21
#1357
Migrate to jni 0.21
#1357
Conversation
In addition to the necessary breaking changes, we also clean up the style to match what is used in the `jni` crate examples.
| @@ -80,16 +80,16 @@ fn print_debug_state() { | |||
| } | |||
|
|
|||
| fn wallet_db<P: Parameters>( | |||
| env: &JNIEnv<'_>, | |||
| env: &mut JNIEnv, | |||
There was a problem hiding this comment.
I'm surprised this wasn't &mut already, given that a JNIEnv can only be used by a single thread.
There was a problem hiding this comment.
After reading jni-rs/jni-rs#392 (comment) I see the issue now; prior to implementing per-frame JNIEnv wrappers with local lifetime, it would have been too restrictive.
| _: JClass<'_>, | ||
| fsblockdb_root: JString<'_>, | ||
| pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_initBlockMetaDb<'local>( | ||
| mut env: JNIEnv<'local>, |
There was a problem hiding this comment.
I was confused by this being mut env rather than &mut env. I guess this section of the jni crate docs partially explains it: if this parameter were &mut env then it would need to be last. But it is not clear to me why that is; or what the trade-offs are between making it mut env, and making it &mut env with reordered parameters.
There was a problem hiding this comment.
Oh, this is the extern function implementing a method, not an independent function that happens to take a Rust JNIEnv as in the section I linked. So the env has to be first, and the jni crate will handle creating a new Rust JNIEnv for the frame. (This actually wraps the same underlying C JNIEnv, which is part of what was confusing me: there are two separate concepts of a JNIEnv at the C and Rust layers.) Got it.
I might submit a PR to the jni crate docs to clarify this, for people who find the description of how JNIEnv is used and then are confused in a similar way.
| } | ||
|
|
||
| fn decode_usk(env: &JNIEnv<'_>, usk: jbyteArray) -> Result<UnifiedSpendingKey, failure::Error> { | ||
| fn decode_usk(env: &JNIEnv, usk: JByteArray) -> Result<UnifiedSpendingKey, failure::Error> { |
There was a problem hiding this comment.
It's kind-of an implementation detail that this doesn't need to call JNIEnv methods that take &mut self. But since it's not public, that doesn't matter I guess.
| @@ -52,7 +52,7 @@ pub fn unwrap_exc_or<T>(env: &JNIEnv, res: ExceptionResult<T>, error_val: T) -> | |||
|
|
|||
There was a problem hiding this comment.
Delete the commented-out code or change it to use env: &mut JNIEnv so that it would compile. (Probably delete it: writing unwrap_exc_or_default is not much more concise than just passing the default, and I prefer the explicitness of the latter.)
| ) -> Result<CommitmentTreeRoot<sapling::Node>, failure::Error> { | ||
| let long_as_u32 = |name| -> Result<u32, failure::Error> { | ||
| fn long_as_u32(env: &mut JNIEnv, obj: &JObject, name: &str) -> Result<u32, failure::Error> { |
There was a problem hiding this comment.
Perhaps these helpers should be in utils, since they're used more than once?
In addition to the necessary breaking changes, we also clean up the style to match what is used in the
jnicrate examples.Author
Reviewer
Footnotes
Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs. ↩
While we aim for automated testing of the SDK, some aspects require manual testing. If you had to manually test
something during development of this pull request, write those steps down. ↩
While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Code coverage can be generated with:
./gradlew checkfor Kotlin modules and./gradlew connectedCheck -PIS_ANDROID_INSTRUMENTATION_TEST_COVERAGE_ENABLED=truefor Android modules. ↩Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit. ↩
In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass. ↩
While the CI server runs the demo app to look for build failures or crashes, humans running the demo app are
more likely to notice unexpected log messages, UI inconsistencies, or bad output data. Perform this step last, after verifying the code changes are safe to run locally. ↩