UBFix: SetMonData

[AreaZeroArven]

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!

[mrgriffin]

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:

1. Use memcpy.
2. 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 u16s... 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.

[mrgriffin]

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.

[AreaZeroArven]

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!