-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add support for MixinExtras expressions #2274
base: dev
Are you sure you want to change the base?
Conversation
…ain types of expression
…tion on completion
# Conflicts: # src/main/resources/messages/MinecraftDevelopment.properties
…process in more detail
26f4759
to
2f977ab
Compare
…lly being used when an instruction in the original method is expected or vice versa.
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.
Well this PR is insane. I don't really know anything about MixinExtras, and can barely follow parts of this PR, and can't follow other parts at all. The code seems fine, it's just going over my head in the more complicated places, but I don't have a problem with trusting you. I did look at (most) of the code in this PR at least, even if I don't really know all of what's going on.
I apologize that the only comments I have are unimportant nitpicks, again I'm going mostly on trust here. Get the tests passing and address these and I'm happy to call this one good to go.
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.
This looks really good.
I don't have much to say about the features added as I'm not familiar with MixinExtras.
Also not sure about those invokeLater { WriteCommandAction[...] }
patterns. It might be the only way to achieve this today, but it might age poorly because of https://youtrack.jetbrains.com/issue/IJPL-53
@@ -432,6 +438,7 @@ | |||
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.folding.MixinObjectCastFoldingBuilder"/> | |||
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.folding.MixinTargetDescriptorFoldingBuilder"/> | |||
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.folding.AccessorMixinFoldingBuilder"/> | |||
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.expression.MEDefinitionFoldingBuilder"/> |
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.
Nitpick: why is this not with the other ME EPs a bit further down?
cacheLock.readLock().lock() | ||
try { | ||
val value = getUserData(key)?.upToDateOrNull | ||
if (value != null) { | ||
return value.get() | ||
} | ||
} finally { | ||
cacheLock.readLock().unlock() | ||
} |
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.
This is not super consequential, but using Kotlin's withLock
extension function avoids that Java try/finally-unlock boilerplate.
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 didn't let me do this for some reason, prompting me to add kotlin stdlib to the classpath, but don't we already have stdlib-jdk8?
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.
Perhaps more to the point kt also has ReentrantReadWriteLock().read { }
and a similar write method.
I think once write actions are allowed outside of the EDT we'll have to make a general sweep of the codebase to look for cases like these where we use |
See Llama's gist for a (slightly outdated) overview of what MixinExtras expressions are. Without IDE support, ME expressions would be really inconvenient to write, more so than other mixin features.
ME expression syntax is support through a custom language in IntelliJ, through the use of language injection. To make this nicer to work with, I have slightly extended the ME expression language. Definitions are prefixed with the
class
keyword, and then a boolean literal for whether they are a type or not. Expressions are surrounded withdo { ... }
. For example:would produce the following injected ME expression file:
This feature is basically done now, the only thing left is to switch away from the JitPack version of MixinExtras and use the library they'll provide for us, which will hopefully have the same interface.
Don't worry about the amount of commits, I'll squash them all on merge.