RIP-Comm / clementine

Gameboy Advance emulator.
MIT License
49 stars 13 forks source link

Move checksum validation into its function #43

Closed simo498 closed 2 years ago

simo498 commented 2 years ago

Should close #28

FedericoBruzzone commented 2 years ago

Can you write a test to see if everything is ok?

simo498 commented 2 years ago

Can you write a test to see if everything is ok?

Let's open an issue for that!

guerinoni commented 2 years ago

The behavior is different, because before of this changes the function it returns a Result that enable future better error handling, right now we have a panic instead... I don't know if this is testable, sure we can add a unit test in this PR if we want but we can't add to the repo a real ROM then...

simo498 commented 2 years ago

I guess we can rethink of the whole function to give back a result if it's needed, i removed it because the initializing function of the cartridge was meant to be infallible (since it's useless to catch an error if the cartridge is invalid)

FedericoBruzzone commented 2 years ago

I think It is better to keep without 'Result' but let's talk about it.

I don't think it will be necessary to have a negative result.

guerinoni commented 2 years ago

I guess we can rethink of the whole function to give back a result if it's needed, i removed it because the initializing function of the cartridge was meant to be infallible (since it's useless to catch an error if the cartridge is invalid)

I know, it was there for future improvement, keep it and add a Result on new function, in this way we can handle correctly from the caller, one day we will add better error handling.

I guess some kind of error are not-acceptable error, like reading in wrong area of memory, and there is a bug from our implementation, other things like checksum or nintendo logo are only nitpicks and we can handle in other way for add a workaround in future, like "checksum is wrong, start anyway?" Because these kind of check were studied for avoid hacking cartridge or fake clone... We don't really care of this :D

simo498 commented 2 years ago

Ok, that's fine 👍