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

Add rkyv serialization #208

Merged
merged 1 commit into from
Aug 17, 2022
Merged

Add rkyv serialization #208

merged 1 commit into from
Aug 17, 2022

Conversation

Kijewski
Copy link
Contributor

No description provided.

@Kijewski
Copy link
Contributor Author

I omitted writing fuzzing tests. We would only test if rkyv can (de)serialize &strs, which would be of little use, IMO. What do you think?

}

#[cfg(test)]
#[cfg_attr(miri, ignore)] // https://github.com/rust-lang/unsafe-code-guidelines/issues/134
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this stacked borrows violation come from our code, or rkyv's?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rkyv. rust-lang/unsafe-code-guidelines#134 (comment)

Actually, I think I see what the problem is in their code. In repr.rs:40-62 they would need to replace self.inline.bytes.as_ptr() with self.inline.bytes[..].as_ptr() to make MIRI understand what is going on. If I don't forget it, I'll try if that fixes the problem.

@ParkMyCar
Copy link
Owner

ParkMyCar commented Aug 17, 2022

I omitted writing fuzzing tests. We would only test if rkyv can (de)serialize &strs, which would be of little use, IMO. What do you think?

Not adding to the fuzzing framework is fine with me, we don't have fuzz coverage for serde either. Would you mind adding a proptest though? You could also convert the existing test if that works

@ParkMyCar
Copy link
Owner

Thanks for the PR! I haven't used rkyv myself but have definitely heard of it, and making CompactString usable with more crates is always good :)

@ParkMyCar ParkMyCar merged commit f87af8f into ParkMyCar:main Aug 17, 2022
@Kijewski Kijewski deleted the pr-rkyv branch August 17, 2022 14:36
@ParkMyCar
Copy link
Owner

I omitted writing fuzzing tests. We would only test if rkyv can (de)serialize &strs, which would be of little use, IMO. What do you think?

Not adding to the fuzzing framework is fine with me, we don't have fuzz coverage for serde either. Would you mind adding a proptest though? You could also convert the existing test if that works

@Kijewski sorry for the quick succession of comments here. Just realized we don't have any tests for serde! I'm going to add some tests now, and will add a proptest for rkyv at the same time

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.

2 participants