Skip to content
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

use serde instead of rustc_serialize #1

Closed
wants to merge 3 commits into from
Closed

use serde instead of rustc_serialize #1

wants to merge 3 commits into from

Conversation

tatref
Copy link

@tatref tatref commented Jan 13, 2018

I wanted to tackle simias/rustation#38, and I thought I'd try on a smaller projet first, so here is a conversion from rustc_serialize to serde.

The only trouble is the following, I had to use a macro to serialize the data field.
I could validate a full serialize/deserialize cycle, because the ROMs I tried panicked with: Unhandled CUE pregap read

Let me know what you think about this PR

@simias
Copy link
Owner

simias commented Jan 26, 2018

Sorry for taking so long to review this, I hadn't checked my github in a while. I'll be sure to look into it in the following days.

Thank you for your interest at any rate!

@simias
Copy link
Owner

simias commented Jan 28, 2018

As far as the code is concerned it looks great to me. I'm slightly annoyed that we still have to use hackery to serialize big arrays but I guess it can't be helped in the current state of Rust and the custom derive is definitely more elegant than my solution.

Do you think the "big array" trait/macro will have to be replicated in every repositories or can it be centralized somewhere? Is it available in some 3rd party crate somewhere?

I couldn't get the full emulator to compile since RustcEncodable/Decodable doesn't seem to be compatible with Serde, I get errors like:

35 | #[derive(RustcDecodable, RustcEncodable)]
   |          ^^^^^^^^^^^^^^ the trait `rustc_serialize::Decodable` is not implemented for `cdimage::sector::Sector

How did you work around this?

Sorry again for taking so much time to look into this.

@tatref
Copy link
Author

tatref commented Jan 28, 2018

Hi,

First off, don't be sorry for taking your time!

Unfortunately, yes, the "big array" hack is still required (as long as the const generics are implemented). I couldn't find it on crates.io, but it would be interesting to put it on a dedicated repository.

The rustc and serde traites are not compatible, all the rustc part of the emulator must be rewritten... I tried it, be couldn't finish it properly. I have issues with the callback! macro, and also some Decodable implemetation that decode multiples fields in one single go.

Thanks for your answer!

@simias
Copy link
Owner

simias commented Jan 30, 2018

Ah I see, I misunderstood your original comment and thought you had managed to get the full emulator running.

So yeah this patch looks great to me but obviously its use is limited as long as the rest of the emulator is ported to serde. Can you maybe submit a PR of your work-in-progress for the rest of the emulator? I could have a look at it and see what can be done.

Regarding the callback! macro as you probably figured out it's just a workaround to be able to serialize function pointers. One possibility would be to get rid of function pointers altogether and instead dispatch the call with a match on some enum state machine. On a modern architecture it might actually end up being faster than the indirect branching too...

@simias simias force-pushed the master branch 2 times, most recently from db65ac2 to 6c74340 Compare September 17, 2022 23:00
@tatref tatref closed this by deleting the head repository Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants