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

Travis build and test fixes #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Travis build and test fixes #9

wants to merge 2 commits into from

Conversation

bonifaido
Copy link
Contributor

Hi Attila,

I did a few simple changes:

  • bumped to a new SNAPSHOT version (0.8-SNAPSHOT is the next I guess)
  • moved asm to test scope (end users don't need this, only the tests afaik)
  • removed openjdk8 from .travis.yml (this version is not available on travis)
    Travis link to the build: https://travis-ci.org/bonifaido/dynalink/builds/47967619

However when running the tests on JDK7(u75) and JDK8(u31) I had encountered some failures.

  • In TestCompositeOperations the Object return type is mandatory so I made the dummy class return the set value.
  • In TestPropertySetter I did the same, however it looked strange to me why a setter needs a return type (?)
  • The interesting part is that TestOverloadedDynamicMethod::testStringFormat works on JDK8 but not on JDK7 because the behavior of MethodHandle.asType() is different between the two VMs. JDK 8 does a fastpath when the types are the same, however JDK7 doesn't, so it returns a new MethodHandle which is different from the original one. I have added a manual fastpath so the tests passed. If this difference is an issue or not, I think I need your guidance on that.

The changes are not for actual pulling just for showing what I tried to accomplish.

- asm moved to test scope, production code isn't using it
- version bumped
… about them, the OverloadedMethod fix concerns me the most
@szegedi
Copy link
Owner

szegedi commented Jan 23, 2015

Hi,

thanks for looking into this. The story here is that over the holidays, I started cross-porting changes from my work on the embedded version of Dynalink in OpenJDK back into this standalone Dynalink version for an eventual 0.8 release. It's been more than a year and I wanted to make sure that standalone version doesn't become obsolete.

It turned out, however, that the changes contain some bugs with regard to handling composite and overloaded operations in presence of void return types. I was fixing some of those in the OpenJDK-embedded version (e.g. http://hg.openjdk.java.net/jdk9/dev/nashorn/rev/d4510be6f97a). So, while your changes to use Object instead of void in test methods do in fact make tests pass, unfortunately they do it by masking the bug :-). The point is that Dynalink must be able to handle void-returning setters! However, fixing this turned out to be less trivial than expected; as you can see, I fixed it in OpenJDK (that fix is actually partial) and will be cross-porting those changes soon.

The difference between JDK7 and JDK8 is interesting… I might need to look into it and see if it's an allowed implementation-specific behavior, or maybe we need to file a bug against JDK7. As you can see in my OpenJDK commit above, I actually removed that asType in OverloadedMethod.java in my fix, as later OverloadedMethod.selectMethod will anyway force the right return type when it invokes SingleDynamicMethod.getInvocation(). That way I pushed the return value conversions to a later time, which actually makes it possible for the autoConversionStrategy set on DynamicLinkerFactory to effect a customized void-to-Object conversion. We can revisit this issue when I cross-ported these latest fixes.

As far as the build and travis related changes, they are fine - thanks; I will gladly accept them. As a matter of legal project governance, I need a signed Contributor Agreement on your behalf in order to accept any changes. You can find the agreement here: https://gist.github.com/szegedi/319eac8cf42fb8f259b8. I'll send you contact information in the private about how you can get a signed copy to me.

@szegedi
Copy link
Owner

szegedi commented Jan 26, 2015

Okay, I cross-ported the fixes for void type handling. Can you submit a new pull request with only the travis and POM changes?

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