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

Support more constant types #11

Merged
merged 7 commits into from
Apr 12, 2021
Merged

Conversation

Daomephsta
Copy link
Collaborator

@Daomephsta Daomephsta commented Apr 8, 2021

Adds support for short, byte, and char simple constants; plus byte and short flag constants.
Boolean simple constant support is not included, as parameter names are adequate for giving true & false meaning.
Character flag constant support is not included, as chars aren't really meant to be used as integers, and only differ from shorts in that they are unsigned. I'm considering it though, Mojang does plenty of weird stuff.

Supersedes #5.

@Daomephsta Daomephsta marked this pull request as ready for review April 8, 2021 09:40
@OroArmor
Copy link

OroArmor commented Apr 9, 2021

Not sure that byte constants are working. Just tested this or with the entity status constants, and it did not work. The other unpicks work, though it could be the unpicks from that that are wrong

@Daomephsta
Copy link
Collaborator Author

@OroArmor
Try again with the latest commits please. I've verified the tests are generating and transforming bytecode as expected. If it's still not working, there might be a bug in the parser.

Copy link

@liach liach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@OroArmor
Copy link

OroArmor commented Apr 9, 2021

Can confirm that this is working with byte constants

@Daomephsta
Copy link
Collaborator Author

Daomephsta commented Apr 10, 2021

I've looked into char flags and decided against them. They're more difficult to implement due to Character not extending Number, a class the flag code depends on a fair bit. It may be possible to remove that dependency, or create an UnsignedShort class to use in place of Character; but I didn't find any char flags in 21w14a, so I vote we deal with that edgecase only if it ever occurs.

@liach
Copy link

liach commented Apr 10, 2021

Yeah, we probably won't use char as integertype. but we should still use them as literal types as they do exist in places like string concatenation, indexOf, etc.

@Daomephsta
Copy link
Collaborator Author

Yeah, we probably won't use char as integertype. but we should still use them as literal types as they do exist in places like string concatenation, indexOf, etc.

char simple constant support has been in this PR since the beginning

@Daomephsta Daomephsta merged commit 792c707 into FabricMC:master Apr 12, 2021
@Daomephsta Daomephsta mentioned this pull request Jan 17, 2025
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.

4 participants