Closed glegris closed 10 years ago
Not sure I like the 1.6 support. 1.6 is kind of outdated IMO and this is a 1.8 JVM... so... what gives?
I see this code as a start point. Implementing a fully working Java 6/7 library already requires a lot work IMHO. New Java 8 methods/classes can be added later. e.g. adding invokedynamic support (required for lambda expressions) is not an easy task for a start. A simple Hello World program in Java 8 will load a lot more classes than in the previous versions. So the implemention task is huge.
You've made a very significant change here so I think we should definitely talk about this for a bit before possibly merging.
My first concern is that this comes from the Avian library. Are there subtle differences which will cause us to have incompatibilities with already established Java frameworks and programs? A goal of mine has been to eventually be able to run the Java compiler inside JVML with as little porting as possible, so it would be nice if we could keep the library working the same way.
My other concern is whether all of this has been tested or not. The VM has come a long way, but we still get unexpected things happening from time to time. Unless you've tested all of this to a reasonable extent, we don't necessarily know if the JIT will handle all of it correctly or if it will even run correctly.
That being said, you've added a lot of functionality here I think, so if possible I would like to merge this. I just want to make sure it's ready to be merged first.
@Yevano Sure, I'll provide the framework to test the implementation behavior. You're right, it's better to not merge before all the tests have been passed. I'll complete the Avian implementation with some well-tested Android (Harmony) code.
I don't like dropping the 1.8 features. That much alone makes this not worth merging IMO.
I'm also not sure how I feel about using so much code that isn't our's. I think I'd rather just use an actual rt.jar than a bunch of source code that doesn't belong to us. ESPECIALLY since we can ruthlessly test our own classes in our unique environment, but this largely doesn't seem to be tested.
Also, next time please use more commits to document your changes. Monolithic commits aren't very nice to review.
@ElvishJerricco No problem but you seemed ok in the discuss here https://github.com/Yevano/JVML-JIT/issues/25 Using the official rt.jar is a also good choice if licensing is not an issue (GPL with a linking exception) So maybe it's better to remove my pull request ;-)
Yea using all this Avian code is tricky... I want it in the project but I don't want so much code that we don't know =P
I think I'd like this pull request if you went back on your java 8 change, and if you implemented the typical ArrayList in addition so that things which import java.util.arraylist have a class available.
In detail about going back on the java 8 change, since this moves us into non-our's code, it'd be best if interfaces in the library match the ones used in a real java 8 library. So using default methods where ordinary java does (there's a couple in java.util.List, for example). And also the code in the cc package should be reverted to using defaults the way it was before.
If you're willing to do these things, I welcome this merge. It takes a huge step forward toward being a more realistic JVM.
Also, @Yevano have you reviewed Avian's license? I haven't, so I'm not sure that we're legally allowed to use, modify, and distribute their code. I'm sure @glegris wouldn't do this if we weren't, but I just want us to be sure.
Just read the top of one of the Avian source files. States very clearly we are allowed =P Cool.
@ElvishJerricco @Yevano
Ok, I'll do these changes:
Are you ok ?
Honestly I think it makes more sense to grab source code from the Java 8 standard library and from there just comment out things which don't currently work and implement natives as best as possible.
Well I like the idea of having the current arraylist implementation being part of a separate package. It's good to have the natives-optimized version available. I'm just saying we should have a java.util.arraylist. Unfortunately, (unless I read their license wrong) oracle won't let us use their code unless it's only used as a reference. We can't copy their code.
@ElvishJerricco If you reuse/modify OpenJDK code, you must release your own code under GPL with a linking exception (a.k.a Classpath exception)
What open-source license is OpenJDK published under? GPL v2 for almost all of the virtual machine, and GPL v2 + the Classpath exception for the class libraries and those parts of the virtual machine that expose public APIs." http://openjdk.java.net/faq/
(Binary) Code from the JDK uses another license What is the Binary Code License? It’s the license Sun has used for the JDK and JRE (and many other Java products). You have to accept the license prior to downloading/installing Java. See http://www.oracle.com/technetwork/java/javase/downloads/jdk-6u21-license-159167.txt for the license text for Oracle JDK 6u21, for example.
Also, should we be concerned about the relatively low default space limit on CC computers? Having all of the standard library implemented might take up too much space. If that's the case I think we need to keep doing what we've been doing, only implementing what's necessary as we go along.
@ElvishJerricco Yeah that's why I started from the Avian library which is known for its very lightweight implementation
The best is to allow to use different standard libraries (as Avian, JamVM or Cacao VM do):
What are your guys' thoughts on default methods in the collections API? Should we be implementing a bunch of them since we can? Usually, not having these defaults leads to a lot of boilerplate code in a lot of collection or list implementations
I think default methods should be embraced. All of Java 8 should be. Going back to my original thing: this JVM is supposed to be 1.8 compatible. The only reason it isn't is invokedynamic.. which can come later. Until then we have to get along without actually using it, but that's easy when we're still working on the VM and library. Makes testing the library hard though...
1.8 Master Race!
Also, in the interest of staying lightweight, I don't think date and time APIs that are completely inapplicable to CC are necessary.
@ElvishJerricco Sure, I understand. I had to import a bunch of code from Avian. From now my commits will be more granular. I updated the Date API to allow user programs to compile/run against it but the implementation is still quite lightweight.
Elvish and I decided we don't like the huge additions to the library without any particular cause. It's just something we're not ready for yet, and it isn't necessary. That said, if you find particular cases where you need some class to implement something, then we shouldn't have a problem.
@Yevano @ElvishJerricco No problem ! :wink: Let me know if you need help in the future.