-
-
Notifications
You must be signed in to change notification settings - Fork 2
Add multi-release optimised methods for LogUtils #10
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
base: master
Are you sure you want to change the base?
Conversation
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.
Main issue is project layout the code looks fine to me.
@@ -14,9 +14,22 @@ version = gitversion.tagOffset | |||
|
|||
println "Version: $version" | |||
|
|||
final java11 = sourceSets.create('java11') | |||
|
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.
SourceSets for multi-release jars dont work in a lot of ides (anything except intellij) so the proper way to do it is to have the java11 code in a sub project.
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.
That is fine. I've seen this done in several different ways so I want to make sure that we just stay consistent moving forward, and I'd like to do it in a way that is as Gradle-friendly as possible.
So, my thinking for the main project is this:
tasks.named('jar', Jar) {
into('META-INF/versions/11') {
from project(':log-utils:java11').tasks.named('compileJava')
}
}
Then, in each of the subprojects, depend on implementation rootProject
.
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 code block is just a sketch, I'd probably use whatever the cleanest solution is. I'm sure there is a way to reference a subproject with respect to the current project (i.e. subprojects['java-11'] = project(':log-utils:java-11')
).
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.
Im fine with whatever way gradle wants it to be done.
I dont know how to do cross project merging.
But because non-intellij (yes, eclipse) IDEs dont separate dependencies/java deps on a sourceset level it is better to split everything into sub-projects.
Similar to how i've split AccessTransformer/ModLauncher/SecureModules (etc) tests into sub projects.
So ya, sub-projects rather then sourcesets if it is a different set of dependencies/java targets.
log-utils/src/java11/java/net/minecraftforge/util/logging/MultiReleaseMethods.java
Show resolved
Hide resolved
This PR is fine, just give me time to review the project setup so I can clean it up and/or take note of it for future projects. My goal is to make all of our projects as similar as possible so we don't run into inconsistencies when dealing with them in IDEs or at build time. |
Ya, I have no issue with the content of the PR, just the project layout. |
Adds multi-release support for LogUtils to use a couple of more optimised methods that aren't available in Java 8.
String#repeat(int)
Should hopefully be self-explanatory... it's a more optimised variant of the StringBuilder approach that takes advantage of internal fields and methods inside the String class.
Map#entry(K, V):
Previously, a CapturedMessage POJO was used. This works fine and is straightforward, but its final fields are not trusted and can't be treated as a Valhalla value class in the future.
By switching to Map.Entry, we can use
new AbstractMap.SimpleImmutableEntry<>(key, value)
on Java 8 which gives us the benefit of trusted final fields (due to its final fields being defined inside JDK internals). On Java 11, we can useMap.entry(key, value)
which additionally has the benefit of being marked with@jdk.internal.ValueBased
, meaning we get the benefits of Valhalla value classes when running on a capable JVM.