Derive Serialize and Deserialize for all IR types #48
Conversation
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an autogenerated code review, new suggestions: 3
The .monocodus config not found in your repo. Default config is used.
Check config documentation here
There was a problem hiding this comment.
This is an autogenerated code review, no new suggestions, fix old one
The .monocodus config not found in your repo. Default config is used.
Check config documentation here
|
The build should pass as soon as a version of |
|
I'm not sure of the best way to handle the Monocodus fmt errors; the commit suggestions from the Monocodus bot produced invalid code. Possible solutions:
|
|
Sorry about that! Please don't mix formatting into the PR, we'll do it separately. Monocodus is a fairly new thing we are testing, and it just got an ability to be configured. I disabled the formatting check for now. |
|
Yeah, good point! I added two tests. |
There was a problem hiding this comment.
Thank you!
I think it looks good now. Just 1.5 concerns there.
Also, let's squash the commits to a state where each commit can individually pass the CI, i.e. you could interactively rebase, and at each point push the commit here, so that CI starts working on it.
test-data/boids-module.ron
Outdated
| name: "atan2", | ||
| arguments: [ | ||
| ( | ||
| index: 5, |
There was a problem hiding this comment.
It looks to me that most of the space in this RON is dedicated to Handle<> serialization.
Would you be open to augment it a bit in the same way wgpu deals with Ids?
See https://github.com/gfx-rs/wgpu/blob/0a0ef9a1000a4d327209570dd2606008b133a2e3/wgpu-core/src/id.rs#L22-L29
In this case, it would need to look like Id(index)
There was a problem hiding this comment.
I gave this a shot but I had trouble getting serde_derive to work with the PhantomData. Do you think it should be possible with serde_derive? Or possibly I could try implementing Serialize and Deserialize by hand for Handle<T>?
There was a problem hiding this comment.
The code I linked to does almost precisely this, it also has PhantomData. Implementing the traits manually is also OK to me, but I think the same approach as linked is slightly easier.
There was a problem hiding this comment.
Yeah, you're right. I'll take another look to see what I am getting wrong.
There was a problem hiding this comment.
All right, I implemented and tested the concise handle serialization, then pushed it onto this PR. Fun trick 😀
|
Let's squash some more, given one of the commits is "Add poor man's snapshot test for serialization". Maybe even squash everything? Would be easier to confirm CI is happy with each commit, this way |
- Add serialize and deserialize Cargo features - Add round-trip serde tests for Handle and Module
|
All right, I squashed into a single commit. BTW in case you missed it due to the long PR history, the build is currently failing because it's waiting on a deploy of gfx-rs/rspirv#123. |
Now the ron representation of a Handle just looks like Handle(42)
|
@paulkernfeld just checking, this is still blocked? |
|
@kvark thanks for checking in! It looks like this is still blocked on a release of |
|
Ah, it's actually now separate |
|
@paulkernfeld Are you still working on this? If not, I'd be happy to take over. |
|
Replaced by #125 |
|
@lachlansneff thanks for picking up the torch on this! I lost track of it. |
Add serialize and deserialize Cargo features, a step towards #32