-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
UBFix: SetMonData #1964
base: master
Are you sure you want to change the base?
UBFix: SetMonData #1964
Conversation
This is so that the values will be aligned and assigned appropriately by SetMonData.
Use these macros where appropriate
Use them appropriately
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.
What aboutSetCurrentBoxMonData
, surely it needs the same treatment? And also arguably SetBoxMonDataAt
for consistency.
EDIT: Actually, I don't think it does, it seems to always be passed a u16 *
.
Two alternative changes to avoid violating strict aliasing rules that could be local to SetMonData
:
- Use
memcpy
. - Use
unsigned char *
.
I don't actually think you've fixed all the implementation-defined behavior, because you're still relying on the values being little endian. I'm not actually sure what UB/implementation-defined behavior you have fixed, could you explain it to me?
EDIT 2: Actually this probably does address the endianness issues--I suspect the data in the battle controllers was explicitly written as little-endian, and everywhere else we're reading through a pointer to the correct type.
That said, it's nice that the u8
arrays have been turned into u16
s... Although it's very unfortunate that the compiler won't give you a warning if any new code erroneously passes a u8
array to SetMonData
(that now takes a u16 *
) because that's something that a user could easily have done via copy and paste.
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.
Because my review was requested:
I think this change is correct based on inspection alone, although there was that SetMonData32
bug, so it's probably not tested?
EDIT: Obviously not quite correct, see the review comment below. (Forgot to come back and edit this sentence before submitting)
I don't love the names of SetMonData16
/SetMonData32
. If I was going to design it myself, I'd maybe do something like:
- SetMonData16(&gEnemyParty[monId], MON_DATA_SPECIES,
&gBattleBufferA[gActiveBattler][3]);
+ u16 data16 = T1_READ_16(&gBattleBufferA[gActiveBattler][3]);
+ SetMonData(&gEnemyParty[monId], MON_DATA_SPECIES, &data16);
Although this is definitely a bit of a trade-off because you wouldn't be able to declare variables at these places in the code.
I've written one review comment that I think represents a bug that needs fixing, besides that this review could be considered an "Approve", but I would caution the person who accepts + merges this PR to give it another scan to ensure there's no other incorrect data types being used.
You are absolutely correct. Let me redefine these macros to use this macro. Thank you! |
SetMonData is cursed.
It is also UB, or implementation defined at least.
Because it works by casting to a u8 and then accessing it like an array, which works, but the proper way would be to have it be a void and treat it based on the type already.
As such, I wrote a few macros so that all that needs to happen is that the right pointer needs to be passed.
As such, I did UBFixes where there are workarounds such as assigning a u16 to a u8 array.
Thank you!