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

Allow enums to implement NativeMapped #1003

Closed

Conversation

koraktor
Copy link
Contributor

This will allow enums to be used inside structures.

Currently (i.e. 4.5.x), it’s impossible to use enums (instead of int) inside a Structure. This will fail in Native.getNativeSize().
This patch will fix this problem and allow enums like:

public TestEnum implements NativeMapped {
    ONE, TWO, THREE;

    @Override
    public Object fromNative(Object nativeValue, FromNativeContext context) {
        return values()[(int) nativeValue];
    }

    @Override
    public Object toNative() {
        return ordinal();
    }

    @Override
    public Class<?> nativeType() {
        return Integer.class;
    }
}

With Java 8’s default methods it’s even possible to completely move this into an own interface, so you’ll only need to add the interface to an enum and be done.

PS: I don’t know which Java version JNA is currently targeting. Should I add such a special enum interface with default methods to this patch?

@matthiasblaesing
Copy link
Member

There is alternative, that does not even need changes to the enums: Use a TypeConverter and register that with the typemapper of the structure/library. I suggest to try that way first.

@koraktor
Copy link
Contributor Author

@matthiasblaesing Sorry for the late response.

Should the TypeConverter approach work with Structures (Data received from a native API, that is)?
Like said before I experienced exceptions in Native.getNativeSize() when I tried this.

But even if it works I think this change would enable users to use an alternative way to handle enums. And with default interface methods this can be slipstreamed a lot.

@matthiasblaesing
Copy link
Member

matthiasblaesing commented Aug 25, 2018

I see the benefit for types you have control over, so I tend to agree merging this. But please also add unittest(s), that verify the correct handling of the Enums. I adjusted the existing test for enum mapping to also cover the structure case (it indeed needed adjusting). See here:

https://github.com/matthiasblaesing/jna/blob/b8d8bf3dfdfe7e56dab0fd33271197837a1d9acd/test/com/sun/jna/TypeMapperTest.java#L246

Please also add a note to CHANGES.md for this feature. Have a look at the other entries as a template.

Default Interfaces are currently out of scope, as the target java version is still 6. I also doubt, that a default interface is a really good idea. C enums don't need to start at 0 and can have arbitrary integer values, so ordinal works often, but not always.

@koraktor
Copy link
Contributor Author

So is this a known bug in JNA 4.5.2 or did it just affect the tests?

The TypeMapper approach works when supplying enums to APIs as a parameter, but fails for enums inside structures returned by APIs.

Errors are like below:

Native size for type "some.Enum" is unknown
	at com.sun.jna.Native.getNativeSize(Native.java:1279)

And this problem can be fixed with my patch.

@matthiasblaesing
Copy link
Member

My change only added a full implementation of the TypeMapper and the corresponding test. The call EnumerationTestLibrary#testStructurePointerArgument handles structure input and output. Could you please create a simple standalone testcase, that shows your problem, maybe I can see the problem.

For your patch I already admitted, that there is value and I'd be willing to merge it, but it requires testing and adjustment of the changelog.

@koraktor
Copy link
Contributor Author

I added a few tests for the code I touched.

Where should I put this inside CHANGES.md? Is this a feature or a bug fix? I would rather call it an improvement.

@matthiasblaesing
Copy link
Member

Merged with slight changes - thank you!

@koraktor
Copy link
Contributor Author

koraktor commented Sep 2, 2018

Great, thanks!

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