linappleii / linapple

Apple 2e emulator
GNU General Public License v2.0
136 stars 61 forks source link

Getting rid of type "BYTE" #184

Closed leesaudan2 closed 11 months ago

akhepcat commented 2 years ago

This... is actually backward.

the whole point of using the BYTE typedef is to ensure that you're always, exactly, 8bits unsigned.

when you change to unsigned char - you can't be sure that you're actually 8 bits across all architectures.

My approach in the use of the uppercase typedefs for variables here is to show that the bitsize is specific, and changes to wordlength breaks things.

Granted, there's lots of work to be done for normalizing across the codebase, and it's true that it would be cleaner in the long run to eliminate the uppercase typedefs in favor of the actual types (uint8_t in this case) but small changes like uint8_t -> uchar can break things in subtle ways.

This gets more tricksy and important when you're mapping native bytes to emulated bytes.

leesaudan2 commented 2 years ago

This... is actually backward. I agree.

the whole point of using the BYTE typedef is to ensure that you're always, exactly, 8bits unsigned.

when you change to unsigned char - you can't be sure that you're actually 8 bits across all architectures.

True. That's why I had BYTE in my branch. After merging into master, I found (with the help of git's automatic merging) that many other parts have changed to unsigned int. So for consistency with other code fragments, I proposed this pull request.

I would prefer uint8_t instead.

leesaudan2 commented 2 years ago

I have changed it to "Uint8", to be consistent with SDL.

maxolasersquad commented 1 year ago

@ akhepcat, @leesaudan2 I'm trying to clear the back-log of MRs. Any of you see any reason why this should be merged?

leesaudan2 commented 12 months ago

@ akhepcat, @leesaudan2 I'm trying to clear the back-log of MRs. Any of you see any reason why this should be merged?

The reasons are stated as before. I don't have anything new to add.

I've noted your recent effort to make it work with GCC13. You may go through the reasons above to see if they are relevant and future-proof.

Anyway, I leave the final decision to you.