jni-rs / jni-sys

Apache License 2.0
55 stars 20 forks source link

Add 'data' getter for the jvalue #6

Closed stanislav-tkach closed 7 years ago

stanislav-tkach commented 7 years ago

Compatibility has been broken in the f075705a9b092bfa5fccc01763ad89d352caeb8d commit, because public field becomes private.

Unfortunately data field is used in the jni-rs crate. I suppose that trace! can be useful, so I propose to add getter for the data field.

sfackler commented 7 years ago

Oops, I was not aware that anyone was using the internal representation. We should just make the field public again then.

FYI, this type is gong to be turned into a "real" union after Rust 1.20 releases, and it'll be undefined behavior to do what that line is doing.

stanislav-tkach commented 7 years ago

I have updated pull request according to your comment.

this type is gong to be turned into a "real" union after Rust 1.20 releases

Untagged unions will be stabilized? Nice, but could you please clarify what exactly will cause undefined behavior? If unions don't implement Debug trait, then it just breaks compilation. Am I missing something?

sfackler commented 7 years ago

It's undefined behavior to read the union on a different type than it was written. Something like this:

value.z = true;
println!("{}", value.j);

You'll need to configure systest to skip that field since it doesn't exist in C: https://github.com/sfackler/rust-jni-sys/blob/master/systest/build.rs

Should just be

cfg.skip_field(|struct, field| {
    struct == "jvalue" && field == "_data"
});
stanislav-tkach commented 7 years ago

Fixed. :sweat_smile:

sfackler commented 7 years ago

Thanks! Sorry about the breakage. Will publish now.