Buttons are only 16 bits so only use uint16_t to prevent wasting 6 bytes #7820
Conversation
|
Seems trivial, but I wouldn't merge this without careful review |
|
It wasnt super careful review but I compiled it and played a level of Mario Kart Super Circuit with an xbox controller and it worked fine. |
|
|
||
| /* Default key binding value - | ||
| * for resetting bind to default */ | ||
| uint64_t def_joykey; | ||
| uint16_t def_joykey; |
There was a problem hiding this comment.
I'm not familiar with RetroArch, but you probably can't just change the structure in a "driver" without causing incompatibility.
I assume the reason why this field is 64 bit now, is for compatibility in the future: avoid breaking compatibility with existing software using these driver interfaces, when more than 16 bit are required.
Edit: Given the response on my other comment, this is indeed a good change. According to the patch author, these are button indices, not masks. And > 65000 buttons are plenty. Implies the original driver was poorly designed.
|
|
||
| if (key_minus == NO_BTN) | ||
| key_minus = joypad_info.auto_binds[ident_minus].joykey; | ||
| if (key_plus == NO_BTN) | ||
| key_plus = joypad_info.auto_binds[ident_plus].joykey; | ||
|
|
||
| if (drv->button(joypad_info.joy_idx, (uint16_t)key_minus)) | ||
| if (drv->button(joypad_info.joy_idx, key_minus)) |
There was a problem hiding this comment.
What is drv->button? Could it be beneficial (now, or for the future) to instead accept the full uint64_t?
For the record, there are devices with more than 16 digital buttons (although there's even devices with more than 64 digital buttons).
There was a problem hiding this comment.
16 bits allows 65536 buttons, not 16, that callback asks "is button X pressed", RetroArch already only supports that many buttons in the input driver as it takes a uint16_t thus the cast.
I think those types where sized wrong to start with.
As for it being obsessive and nit picky, I agree with you, but anything less then perfect is still wrong, just because you have the extra memory doesnt mean you should waste it.
Also all out devices dont have gigs of memory, RetroArch runs on the PSP which has as low as 32mb.
There was a problem hiding this comment.
I agree. Given the new facts, this type was oversized.
I still wouldn't argue with "wasting memory" (even for platforms like PSP), but simply using the wrong tool for the job (battering ram to hammer a nail).
This comment was marked as spam.
This comment was marked as spam.
|
Alright, let's merge this then. |
Reclaim a little extra memory