Skip to content
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

Direct Mapping for boolean arrays #816

Merged
merged 2 commits into from
May 27, 2017
Merged

Direct Mapping for boolean arrays #816

merged 2 commits into from
May 27, 2017

Conversation

ncruces
Copy link
Contributor

@ncruces ncruces commented May 24, 2017

This is consistent with how these are handled in interface mapping.

Also boolean is ffi_type_uint32 everywhere in the source code, except here.

@ncruces ncruces mentioned this pull request May 24, 2017
@matthiasblaesing
Copy link
Member

In principal I agree with the direction that was started here, but it collides with the current size definition of boolean in JNA (it is defined to 4 bytes). See:

https://github.com/java-native-access/jna/blob/master/src/com/sun/jna/Native.java#L1296

This would not be in line with a 1 byte wide mapping.

There is a request for supporting C99 bool:

#721

I started some work on this, but it is still unready and I think in the wrong direction:

https://github.com/matthiasblaesing/jna/tree/boolean

I think a way could be:

This should go into the 5.0.0 branch, as it invalidates assumptions about sizes and mappings.

@ncruces
Copy link
Contributor Author

ncruces commented May 25, 2017

Yes, I understand it does, and I'm not necessarily very happy with the change. Still two changesets so...

8f2e773 IMHO is a simple fix.
Probably doesn't make much of a difference, but if it does, this is the only place where boolean is signed.

ba7881f simply makes direct mapping consistent with interface mapping.
Consistently wrong? Maybe, but consistent.

Also, note that the same reasoning applies to both boolean and char arrays:

  • char maps to wchar_t, char[] already maps to jchar* (always 16-bit unsigned);
  • boolean maps to uint32_t, boolean[] already maps to jboolean* (always 8-bit unsigned).

There's quite a lot of effort to handle the difference for String, but no such effort was made for char[].

So the only change here is making boolean[] work for direct mapping. I'm not even using this (likely no one is) so feel free to ignore it. But it is how it already works for interface mapping, and it seemed trivial enough.


Now, this is just my opinion, and I'm very, very new to JNA.

But in a sense, I feel JNA already tries to do a bit too much, and would avoid having it do even more. When things are passed by reference, I can do the marshaling by myself (e.g. convert between encodings). It's only when passing arguments and return values by value that JNA must help (with JNI and the ABI), so it doesn't at all bother me that arrays are not marshaled where simple values are.

I'll have a look at your boolean branch. But feel free to ignore this if I'm slowing you down.

@matthiasblaesing
Copy link
Member

Please: No need to apologize, in fact I'm glad your are looking into this! Your points are well taken and your reasoning is sound.

Two things I'd like to ask you: Please add an entry to CHANGES.md. For the first PR I did that:

https://github.com/java-native-access/jna/blob/master/CHANGES.md

The second: Please rebase your changes on top of JNA master (I just merged your first PR)

@ncruces
Copy link
Contributor Author

ncruces commented May 26, 2017

Regarding your branch, using stdbool.h (and custom mapping other boolean types) seems reasonable to me.

Any inconsistencies between jboolean/bool won't be much different from inconsistencies between jchar/wchar_t. It's unfortunate that C was late to standardize these things, but C99 was 18 years ago, and if there's ever going to be any standard boolean type, stdbool.h is it.

The real issue, I think, is breaking compatibility with previous JNA versions, and the mountain of APIs returning an int that would better map to a Java boolean. Then you can go the other way: keep things as they are, come up with a NativeBool mimicking NativeLong?

@matthiasblaesing matthiasblaesing merged commit a6c2083 into java-native-access:master May 27, 2017
@matthiasblaesing
Copy link
Member

@ncruces I merged this PR. Thanks for looking into this. I'll look into rebuilding the pre-build native parts in the next few days.

With regard to StdBool I think the only realistic option is the introduction of a NativeBoolean. At some point I'll look into this again, but not in the near future :-)

@ncruces ncruces deleted the boolean-fixes branch July 20, 2017 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants