-
Notifications
You must be signed in to change notification settings - Fork 31
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
jni_mh.h with jint as 32-bit long #198
Comments
Thanks for taking a look at this and for the detailed context in your bug. Also, please be aware of #197 which first mentions this. I believe the issue is arising here: proxy.h and its counterpart proxy_definitions.h describe the mappings that Unfortunately, if these two types are the same, it's going to complicate what that declaration should look like. The compiler won't be able to differentiate between the two. i.e.
On the one hand, it's good I'm not sure what's right here. I think a syntax like follows makes sense: static constexpr jni::Class { "kClass",
jni::Method { "intM", jni::Return<void>{}, Params { jint{} } },
jni::Method { "longM", jni::Return<void>{}, Params { Type<jlong>{} } },
jni::Method {
"overloadM",
jni::Overload { jni::Return<void>{}, Params { Type<jint>{} } },
jni::Overload { jni::Return<void>{}, Params { Type<jlong>{} } },
},
};
LocalObject<kClass> obj{};
obj("intM", 1); // Fine
obj("intM", jlong{1}); // Impossible to prevent, won't compile on Linux, doesn't shorten or widen
// obj("intM", jlong{BIG_VAL}); // Compiles *nowhere*, would cause shortening.
obj("intM", jint{1}); // Compiles everywhere.
obj("longM", 1); // Impossible to prevent, compiles everywhere.
obj("longM", jlong{1}); // Compiles everywhere.
obj("longM", jlong{BIG_VAL}); // Compiles Linux only, Windows complains of shortening.
// obj("overloadM", 1); // FAILS on *Windows only*, ambiguous.
obj("overloadM", jint{1}); // FAILS on *Windows only*, ambiguous.
// obj("overloadM", jlong{1}); // FAILS on *Windows only*, ambiguous.
// obj("overloadM", jlong{BIG_VAL}); // FAILS on *Windows only*, ambiguous. I actually am not sure what the best way around this is. I'm definitely open to a different syntax than above, but, JNI Bind relies heavily on making inferences from types. If those types become ambiguous, thins become tricky. The above is basically forcing no implicit conversions. I'd also be open to having some kind of "ambiguity ranking" where things work without ambiguity, but anything ambiguous that isn't the default (e.g. int vs long) requires being explicit. What do you think? |
I think You could do a syntax like Ideally this code should compile as-is without further decoration.
Only one method matches on all platforms, regardless of how the jdk typedefs |
Hmm that's strange you're seeing that collision there if the types are different. Wasn't the entire issue that it was being defined as 32 bit but only on Windows? Are you sure it's the same two types colliding? ----- edited to remove a minor confusion I had I agree that using The problem is, that if there's a platform where Perhaps if some arbitrary order is imposed on This has the downside that folks can now compile something that basically doesn't work (if you pass an |
No,
And Windows defines it as:
If I take a big hammer and make the typedef to a C
Nothing material changes, because both C But without that change, jni-bind fails to compile with the gist from earlier. In all universes, both Linux and Windows, this code below will assert regardless of whether it is
There is no platform where |
I took this a little further, but couldn't take it home. Some ascii art annotations on the error message inline commented with
If we can explain where that "extra" [*edit I noticed after I posted that |
Here is the culprit:
Which is probably what you were trying to say earlier. A As to the
Right now this fails to compile. |
Thanks for the super detailed bug descriptions. I think I understand what's up, it may take me a bit to get a fix in place, I'll try to get back ASAP. |
This is potentially causing issues for Windows builds. See #198 for context. PiperOrigin-RevId: 568324793
This is potentially causing issues for Windows builds. See #198 for context. PiperOrigin-RevId: 568324793
This is potentially causing issues for Windows builds. See #198 for context. PiperOrigin-RevId: 569240465
Apologies for taking so long, I did try to dig into this further this weekend but frustratingly, getting a setup to repro the issue has been tricky. Long story short, I tried forcing the types as you've described above, but couldn't repro on my Linux box, probably because of subtleties with other types. I then tried to put together a Windows box with MSVC, but realized I was wasting time putting together environment so I gave up, and tried to compile JNI Bind with Godbolt. This got further, but I started to run into independent issues just compiling JNI Bind at all (and the issues are occurring well before the above failure). E.g. I've been slowly knocking out these other issues (they will need to be fixed anyways), but, what are you compiling JNI Bind with so that it get's close on Windows? My gut tells me that adding Sorry for the delay, |
It won't compile with MSVC. Environment is llvm/clang x86_64-pc-windows-msvc aka My local fix for what it is worth, is this at here:
|
I got jni-bind working on Windows but needed to work-around a difference in how Oracle JDK 19 defines
jint
.On Linux:
On Windows (comment from the JDK preserved):
If I compile on Windows, I get TMP errors:
Full gist here.
I'm not above changing the
typedef
in the JDK, but if it is possible to make jni-bind tolerant of the types encapsulated byjni_md.h
that would be even better.Either way, for my own edification, what I don't entirely grok is why the template specialization is failing to begin with. I don't see it. The
jlong
type is on Windows is:There is no actual difference in the types on Windows and Linux. The types are unique (32-bit and 64-bit literals). Yet
AllUnique<>
fails.The text was updated successfully, but these errors were encountered: