eejbyfeldt commented on code in PR #570:
URL: https://github.com/apache/datafusion-comet/pull/570#discussion_r1642344092
##########
core/src/jvm_bridge/mod.rs:
##########
@@ -236,7 +236,7 @@ impl JVMClasses<'_> {
JVM_CLASSES.get_or_init(|| {
// A hack to make the `JNIEnv` static. It is not safe but we don't
really use the
// `JNIEnv` except for creating the global references of the
classes.
- let env = unsafe { std::mem::transmute::<_, &'static mut
JNIEnv>(env) };
+ let env = unsafe { std::mem::transmute::<&mut JNIEnv<'_>, &mut
JNIEnv<'_>>(env) };
Review Comment:
I find this version easier to understand what is going on
```suggestion
let env = unsafe { std::mem::transmute::<&mut JNIEnv, &mut
JNIEnv<'static>>(env) };
```
##########
core/src/jvm_bridge/mod.rs:
##########
@@ -183,7 +183,7 @@ pub fn get_global_jclass(env: &mut JNIEnv, cls: &str) ->
JniResult<JClass<'stati
// A hack to make the `JObject` static. This is safe because the global
reference is never
// gc-ed by the JVM before dropping the global reference.
- let global_obj = unsafe { std::mem::transmute::<_,
JObject<'static>>(global.as_obj()) };
+ let global_obj = unsafe { std::mem::transmute::<&JObject<'_>,
JObject<'_>>(global.as_obj()) };
Review Comment:
To me the transmute cast from a reference type to a none reference type
looks very fishy to me. But I guess this change just make it more explicit that
this is what is going on.
If I change this whole method to
```
/// Gets a global reference to a Java class.
pub fn get_global_jclass<'a>(env: &mut JNIEnv<'a>, cls: &str) ->
JniResult<JClass<'a>> {
env.find_class(cls)
}
```
the code still builds and seems to test. I guess that is a strong indication
that we do not need this hack.
Created PR removing the method here:
https://github.com/apache/datafusion-comet/pull/580
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
