Add toString to classes extending ByReference#1182
Add toString to classes extending ByReference#1182dbwiddis wants to merge 15 commits intojava-native-access:masterfrom
Conversation
5d377ac to
eb2e2dc
Compare
|
I think the test is failing due to a bug in |
eb2e2dc to
4b3b7ee
Compare
e1a68a0 to
36b9389
Compare
36b9389 to
a579e65
Compare
|
Hmm, ULONGLONG's |
7384829 to
4360379
Compare
4360379 to
0580fa2
Compare
|
Finally, all the tests pass. Ready for review! |
matthiasblaesing
left a comment
There was a problem hiding this comment.
Thank you - this looks sane - however I left some comments inline, which might be a good addition. Please have a look.
|
The toString for NativeLong fails on platforms where native long is 32bit. This is the case on windows (see the appveyor build: https://ci.appveyor.com/project/matthiasblaesing/jna/builds/32826833#L940) and is also reproducible on 32bit JDKs on linux. The hex representation seems to be 64 bit wide. |
|
Well, that's a weird one. Originally I had an if-else because I was duplicating the value, and it worked. With only one value I shortened it to the ternary operator. But apparently Output: So I guess I'll go back to the if-else. |
|
(Also I've now learned about compile-time polymorphism. Yay.) |
|
Could you have a look at this: matthiasblaesing@4b24046? This fetches the value via reflection and thus will not need modifications to the implementation. I try to keep changes to the API to a minimum. |
|
Interesting approach! I like it. Three comments:
|
|
Thank you. For the build/unittest failure: Multicatch is not supported on JDK 6 (yes we are that backwards compatible). For the performance standpoint: toString() was asked to be overriden to make debugging easier. If that becomes a performance bottleneck, we need to reevaluate, but I doubt it. The broad exception catch was done to make the code more readable (see first paragraph). The boxing of primitives is also present in the original implementation - the call to |
|
Oh and another unittest failure:
|
|
Thanks for the compatibility note. I was confused by the test failures! Will fix. |
|
I'm confused on the PACL failure, as I thought I'd fixed that a while ago. The current code is only supposed to test ACL: |
|
And I don't think the existing test failures have anything to do with this code. Seeing them on other unrelated PRs too. |
|
Rebased and merged - thank you. |
Fixes #1179