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
Arm64Emitter: Convert ShiftType to enum class #9446
Arm64Emitter: Convert ShiftType to enum class #9446
Conversation
e134e24
to
d2ae7d6
Compare
|
You can (probably) ignore the Windows fail, thats on our CI...but the Android one needs to be looked at. |
d2ae7d6
to
379a71f
Compare
|
I'm not sure why Visual Studio let that compile in the first place, but I've added the cast. |
Source/Core/Common/Arm64Emitter.h
Outdated
| ST_LSR = 1, | ||
| ST_ASR = 2, | ||
| ST_ROR = 3, | ||
| LogicalShiftLeft = 0, |
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.
I think it'd still be informative as LSL, LSR, ASR, and ROR as the names for the shift types. These are what they're referred to as by ARM in official documentation, and the long-form makes code (imo) slightly more verbose to read and write.
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.
Given that you've made roughly half the commits touching Arm64Emitter in the last few years, I'm especially inclined to defer to your judgement. I've reverted the naming changes.
Said changes were inspired by my not knowing what the terms referred to until I did some googling. In order to help anyone else in a similar situation I've added some comments in the declaration with the full name, which at least in Visual Studio appear in the hover tooltip for the respective enum values.
379a71f
to
1b1357d
Compare
1b1357d
to
e323766
Compare
Straightforward conversion of ShiftType from enum to enum class.