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.

Sign up for GitHub

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

Jump to bottom

Structure.newInstance should be generified so cast isn't necessary #889

Closed
ghost opened this issue Dec 13, 2017 · 7 comments
Closed

Structure.newInstance should be generified so cast isn't necessary #889

ghost opened this issue Dec 13, 2017 · 7 comments

Comments

@ghost
Copy link

ghost commented Dec 13, 2017 •
edited by ghost
Loading 

  array[i] = (Complex) Structure.newInstance(Complex.class, new Pointer(Pointer.nativeValue(p) + i * SIZE));

the cast shouldn't be necessary, the method definition should be
public static <T extends Structure> T newInstance(Class<T> type, Pointer init) throws IllegalArgumentException { ... }
matthiasblaesing added a commit to matthiasblaesing/jna that referenced this issue Dec 23, 2017
@matthiasblaesing
Tighten the generic bounds for Structure#newInstance
99353fe 
The signature was: `Structure newInstance(Class<?> type)` requiring
an explicit cast after the call. The the new signature introduces a 
type parameter `T` with an upper bound of `com.sun.jna.Structure`: 

<T extends Structure> T newInstance(Class<T> type)`

The companion, that takes an init pointer was also updated:

<T extends Structure> T newInstance(Class<T> type, Pointer init)

Closes: java-native-access#889
@matthiasblaesing matthiasblaesing mentioned this issue Dec 23, 2017
Merged
matthiasblaesing added a commit to matthiasblaesing/jna that referenced this issue Dec 23, 2017
@matthiasblaesing
Tighten the generic bounds for Structure#newInstance
a9d7754 
The signature was: `Structure newInstance(Class<?> type)` requiring
an explicit cast after the call. The the new signature introduces a 
type parameter `T` with an upper bound of `com.sun.jna.Structure`: 

<T extends Structure> T newInstance(Class<T> type)`

The companion, that takes an init pointer was also updated:

<T extends Structure> T newInstance(Class<T> type, Pointer init)

Closes: java-native-access#889
@matthiasblaesing
Copy link
Member

Please have a look at #898. I'd like to have a few more opinions on this before it is merged. As the change is binary-incompatible it can only go into the master branch and won't be backported into 4.5.1-dev.

@ghost
Copy link
Author

ghost commented Dec 23, 2017 •
edited by ghost
Loading

Thanks. I thought the whole deal was that the source was not compatible but that the binary generated was in some sense or the other, I could be mistaken about that though. This post at https://stackoverflow.com/questions/2721546/why-dont-java-generics-support-primitive-types seems to agree with the backwards compatible idea though. The code looks good although I think the refactoring should be done so that going from without a cast to a cast in the diff to
private Class<?> getNativeType(Class<?> cls) { ..
in src/com/sun/jna/CallbackReference.java could be avoided by making it
private <T> Class<T> getNativeType(Class<T> cls) { ..
or maybe
private <T> Class<? extends T> getNativeType(Class<? extends T> cls) { .. same goes for the diff to src/com/sun/jna/Function.java , src/com/sun/jna/Native.java, src/com/sun/jna/Pointer.java but I haven't checked into the feasibility of that

matthiasblaesing added a commit to matthiasblaesing/jna that referenced this issue Dec 24, 2017
@matthiasblaesing
Tighten the generic bounds for Structure#newInstance
a9a2c43 
The signature was: `Structure newInstance(Class<?> type)` requiring
an explicit cast after the call. The the new signature introduces a 
type parameter `T` with an upper bound of `com.sun.jna.Structure`: 

<T extends Structure> T newInstance(Class<T> type)`

The companion, that takes an init pointer was also updated:

<T extends Structure> T newInstance(Class<T> type, Pointer init)

Closes: java-native-access#889
@matthiasblaesing
Copy link
Member

You are right for Structure#newInstance - this does not break binary compatibility. This won't have a practical impact though. I have 4.5.1-dev only open for bugfixes and feature development is done on master.

For your comment about "refactoring": I'd really get the casts down. Could you suggest code changes?

@ghost
Copy link
Author

ghost commented Dec 28, 2017

Sure thing, let me get the code checked out.. give me a little bit

@ghost
Copy link
Author

ghost commented Dec 30, 2017

I haven't had time to check the code out yet, but what you can do, if you are running Eclipse, which I highly recommend if not, is to right-click on the newInstance method and click on the References dropdown/context menu, then click Workspace. Basically, just let the compiler guide you thru the source tree

@matthiasblaesing
Copy link
Member

Eclipse does not more than any other IDE at this point. In netbeans it is called "Find usages", but that does not solve the "cast problem" and I assume IDEA et al. offer the same.

@matthiasblaesing
Copy link
Member

@crowlogic I pulled in the PR, feel free to suggest optimization for the internal casts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@matthiasblaesing