-
Notifications
You must be signed in to change notification settings - Fork 227
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
Default methods #30
Default methods #30
Conversation
This fails when merged with master. The version in master processes Retrolambda with itself, so that the Maven plugin (which is Java 6 compatible) can use Retrolambda directly.
Does this patch support the use of 3rd-party interfaces with default methods? How about using them as lambdas? That might explain the above failure. |
return tmp; | ||
} | ||
|
||
static int getReturnIns(Type arg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods getReturnIns and getVarIns can be replaced with org.objectweb.asm.Type#getOpcode
@@ -52,7 +52,7 @@ | |||
</execution> | |||
</executions> | |||
<configuration> | |||
<minimizeJar>true</minimizeJar> | |||
<!-- <minimizeJar>true</minimizeJar> --> <!-- http://jira.codehaus.org/browse/MSHADE-174 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to re-enable minimizeJar after this is merged with master, because the latest Retrolambda backports itself to Java 6.
I've committed IntelliJ IDEA's code style settings to master. Please reformat your code with them (or at least use indent of 4 spaces). |
It will not process library interfaces right now. And i wonder how it would 2014-08-25 18:01 GMT+02:00 Esko Luontola notifications@github.com:
|
It should just ignore library interfaces, because it won't be able to add methods to library interfaces. Especially it's impossible to mask classes in the java.* packages, because the JVM disallows those packages in the user classpath. The program will then at runtime use an older version of for example java.lang.Iterable, which had a bunch of default methods added in Java 8. (Later we can consider copying the default method implementations to the classes, but that might cause more problems than it solves, and anyways nobody can call the method implementation through the interface unless the interface also has the method.) Retrolambda's main method will exit if it's not running under Java 8 and also the Maven plugin will not try running Retrolambda if Maven is not running under Java 8. The Maven plugin just needs to be compilable against Retrolambda, so that we don't have to use reflection. |
Also it's a good benchmark that Retrolambda can backport itself, because it uses lambdas and quite many Java 8 APIs. :) |
I pulled these commits to the https://github.com/orfjackal/retrolambda/tree/default-methods branch and adjusted the code style for DefaultMethodsTest. I'll do some bigger refactoring on the tests a bit later. |
Seems that this pull request is not working even before merging it with master. This happens on OS X.
|
Strange. I did development on osx, unfortunately i've been busy for a Do you have any input on how to handle this case? Should we introduce Best regards
|
The error might be because of something like iterating the classes in different order. File systems can list the files in any order. It's not possible to assume anything about whether the interface or its implementors are discovered first. |
Retrolambda already walks the inputDir file tree. It should backport only the default methods that it finds in the interfaces in that directory. Since the implementors may be visited before or after the interface they implement, it may be necessary to either (1) first find all the interfaces to backport and after that backport all lambdas, or (2) keep track of what interfaces the already visited classes implement and revisit them after it's discovered that some of the interfaces had default methods. Option 1 seems easier to get right. |
I've now refactored DefaultMethodsTest and added some more tests. See the commit bc99e75 in the default-methods branch. |
And now there are still some more tests. I think most of the edge cases are now covered. Or maybe also the method visibility (in implementing classes) needs some checks... I'll need to check how Java 8 handles those cases - it might not compile, but might still be binary compatible (i.e. implementing class has a non-public method with same signature as a default method that was added to an interface). |
I merged this to master using feature toggles. Let's continue discussion at issue #31 |
No description provided.