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.

Sign up for GitHub

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

Jump to bottom

Make [Try]IntoCtx borrow instead of copying #47

Closed
wants to merge 4 commits into from
Closed

Make [Try]IntoCtx borrow instead of copying #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
749f752
Borrow instead of copying in IntoCtx, TryIntoCtx, Cwrite, Pwrite
willglynn Nov 1, 2018
ad6df6e
Add a test which complains about unaligned field accesses
willglynn Nov 2, 2018
2a73c26
Serialize #[repr(packed)] structs by copying out their fields
willglynn Nov 2, 2018
efc16f3
Change iowrite() to also take a reference
willglynn Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ fn write_io() -> Result<(), scroll::Error> {
let mut bytes = [0x0u8; 10];
let mut cursor = Cursor::new(&mut bytes[..]);
cursor.write_all(b"hello")?;
cursor.iowrite_with(0xdeadbeef as u32, BE)?;
cursor.iowrite_with(&(0xdeadbeef as u32), BE)?;
Copy link
Owner

Choose a reason for hiding this comment

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

This and the 3 others are a huge PR, will take me a bit to review, appreciate your patience, @willglynn.
I would like to note that having to take the reference now of variables, literals, makes me pretty sad...
I feel like it jars slightly with the original and basic purpose of scroll, which was to "serialize" numbers and PODs.
I do wish there had been a little more discussion before this enormous (and amazing PR!), specifically, whether we can have our cake and eat it too w.r.t. TryInto for the primitives.
Specifically, I'm wondering if there's a compromise, so that we can still pass either references or owned values to the pwrite/iowrite.
Maybe it's just me but i do find it a usability paper cut and find it less attractive that I have to take references to pwrite now, but perhaps I'm used to the old way.
So, is there some generic magic we can use to do this? Guessing wildy and randomly:

  1. Have someone take AsRef
  2. Have scroll_derive derive the reference versions for any user type (i'm ok with taking references of user structs)
  3. ???

On the other hand, in my case where the struct was too large to be put on the stack, and I had to box it, even if we did say 2., this would still probably eventually blow the stack? Or perhaps not. Have to think about it, and its late and a Thursday :)
Similarly, and against my original sadness, if I'm able to pwrite out a huge boxed struct that auto-derives pwrite, that would actually make me happy and would counteract any bitterness of no longer being able to pwrite raw deadbeefs :D
Dunno, just some thoughts, feel free to respond whenever you get the time (and that goes for anyone else who's watch).
Regardless, thanks for your amazing work here @willglynn and the robustness and tenacity of the changes, great work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers! And yeah, this is why I went and did these PRs even with limited discussion: I didn't know how this concept would look until it was done 😄

I'll ponder the generics question. There might be a way we can make pwrite() be &-agnostic while keeping TryIntoCtx take &self – that'd let you pwrite() either by value or by reference without consuming self during the write.

assert_eq!(cursor.into_inner(), [0x68, 0x65, 0x6c, 0x6c, 0x6f, 0xde, 0xad, 0xbe, 0xef, 0x0]);
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion scroll_derive/examples/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ fn main () {
println!("data: {:?}", &data);
assert_eq!(data.id, 0xdeadbeefu32);
let mut bytes2 = vec![0; ::std::mem::size_of::<Data>()];
bytes2.pwrite_with(data, 0, LE).unwrap();
bytes2.pwrite_with(&data, 0, LE).unwrap();
let data: Data = bytes.pread_with(0, LE).unwrap();
let data2: Data = bytes2.pread_with(0, LE).unwrap();
assert_eq!(data, data2);
Expand Down
120 changes: 84 additions & 36 deletions scroll_derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,38 @@ extern crate syn;

use proc_macro::TokenStream;

fn is_repr_packed(ast: &syn::DeriveInput) -> bool {
use proc_macro2::{Delimiter,TokenTree};

ast.attrs.iter()
.find(|attr| {
// find an Attribute with ident == "repr"
attr.path.segments.len() == 1 &&
&attr.path.segments[0].ident.to_string() == "repr"
})
.map(|repr| {
// if found, see if it's "(packed)"
// this doesn't seem ideal, but it does work
match repr.tts.clone().into_iter().next() {
Some(TokenTree::Group(group)) => {
if group.delimiter() == Delimiter::Parenthesis {
match group.stream().into_iter().next() {
Some(TokenTree::Ident(ident)) => {
&ident.to_string() == "packed"
}
_ => false,
}
} else {
false
}
},
_ => false,
}
})
// in the absence of #[repr], we're not packed
.unwrap_or(false)
}

fn impl_struct(name: &syn::Ident, fields: &syn::FieldsNamed) -> proc_macro2::TokenStream {
let items: Vec<_> = fields.named.iter().map(|f| {
let ident = &f.ident;
Expand Down Expand Up @@ -70,45 +102,51 @@ pub fn derive_pread(input: TokenStream) -> TokenStream {
gen.into()
}

fn impl_try_into_ctx(name: &syn::Ident, fields: &syn::FieldsNamed) -> proc_macro2::TokenStream {
fn impl_try_into_ctx(name: &syn::Ident, fields: &syn::FieldsNamed, is_packed: bool) -> proc_macro2::TokenStream {
let items: Vec<_> = fields.named.iter().map(|f| {
let ident = &f.ident;
let ty = &f.ty;
match *ty {
syn::Type::Array(_) => {
quote! {
for i in 0..self.#ident.len() {
dst.gwrite_with(&self.#ident[i], offset, ctx)?;
if is_packed {
quote! {
for i in 0..self.#ident.len() {
dst.gwrite_with(&{self.#ident[i]}, offset, ctx)?;
}
}
} else {
quote! {
for i in 0..self.#ident.len() {
dst.gwrite_with(&self.#ident[i], offset, ctx)?;
}
}
}
},
_ => {
quote! {
dst.gwrite_with(&self.#ident, offset, ctx)?
if is_packed {
quote! {
dst.gwrite_with(&{self.#ident}, offset, ctx)?
}
} else {
quote! {
dst.gwrite_with(&self.#ident, offset, ctx)?
}
}
}
}
}).collect();

quote! {
impl<'a> ::scroll::ctx::TryIntoCtx<::scroll::Endian> for &'a #name {
impl ::scroll::ctx::TryIntoCtx<::scroll::Endian> for #name {
type Error = ::scroll::Error;
#[inline]
fn try_into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian) -> ::scroll::export::result::Result<usize, Self::Error> {
fn try_into_ctx(&self, dst: &mut [u8], ctx: ::scroll::Endian) -> ::scroll::export::result::Result<usize, Self::Error> {
use ::scroll::Pwrite;
let offset = &mut 0;
#(#items;)*;
Ok(*offset)
}
}

impl ::scroll::ctx::TryIntoCtx<::scroll::Endian> for #name {
type Error = ::scroll::Error;
#[inline]
fn try_into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian) -> ::scroll::export::result::Result<usize, Self::Error> {
(&self).try_into_ctx(dst, ctx)
}
}
}
}

Expand All @@ -118,7 +156,7 @@ fn impl_pwrite(ast: &syn::DeriveInput) -> proc_macro2::TokenStream {
syn::Data::Struct(ref data) => {
match data.fields {
syn::Fields::Named(ref fields) => {
impl_try_into_ctx(name, fields)
impl_try_into_ctx(name, fields, is_repr_packed(&ast))
},
_ => {
panic!("Pwrite can only be derived for a regular struct with public fields")
Expand Down Expand Up @@ -264,48 +302,58 @@ pub fn derive_ioread(input: TokenStream) -> TokenStream {
gen.into()
}

fn impl_into_ctx(name: &syn::Ident, fields: &syn::FieldsNamed) -> proc_macro2::TokenStream {
fn impl_into_ctx(name: &syn::Ident, fields: &syn::FieldsNamed, is_packed: bool) -> proc_macro2::TokenStream {
let items: Vec<_> = fields.named.iter().map(|f| {
let ident = &f.ident;
let ty = &f.ty;
let size = quote! { ::scroll::export::mem::size_of::<#ty>() };
match *ty {
syn::Type::Array(ref array) => {
let arrty = &array.elem;
quote! {
let size = ::scroll::export::mem::size_of::<#arrty>();
for i in 0..self.#ident.len() {
dst.cwrite_with(self.#ident[i], *offset, ctx);
*offset += size;
if is_packed {
quote! {
let size = ::scroll::export::mem::size_of::<#arrty>();
for i in 0..self.#ident.len() {
dst.cwrite_with(&{self.#ident[i]}, *offset, ctx);
*offset += size;
}
}
} else {
quote! {
let size = ::scroll::export::mem::size_of::<#arrty>();
for i in 0..self.#ident.len() {
dst.cwrite_with(&self.#ident[i], *offset, ctx);
*offset += size;
}
}
}
},
_ => {
quote! {
dst.cwrite_with(self.#ident, *offset, ctx);
*offset += #size;
if is_packed {
quote! {
dst.cwrite_with(&{self.#ident}, *offset, ctx);
*offset += #size;
}
} else {
quote! {
dst.cwrite_with(&self.#ident, *offset, ctx);
*offset += #size;
}
}
}
}
}).collect();

quote! {
impl<'a> ::scroll::ctx::IntoCtx<::scroll::Endian> for &'a #name {
impl ::scroll::ctx::IntoCtx<::scroll::Endian> for #name {
#[inline]
fn into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian) {
fn into_ctx(&self, dst: &mut [u8], ctx: ::scroll::Endian) {
use ::scroll::Cwrite;
let offset = &mut 0;
#(#items;)*;
()
}
}

impl ::scroll::ctx::IntoCtx<::scroll::Endian> for #name {
#[inline]
fn into_ctx(self, dst: &mut [u8], ctx: ::scroll::Endian) {
(&self).into_ctx(dst, ctx)
}
}
}
}

Expand All @@ -315,7 +363,7 @@ fn impl_iowrite(ast: &syn::DeriveInput) -> proc_macro2::TokenStream {
syn::Data::Struct(ref data) => {
match data.fields {
syn::Fields::Named(ref fields) => {
impl_into_ctx(name, fields)
impl_into_ctx(name, fields, is_repr_packed(&ast))
},
_ => {
panic!("IOwrite can only be derived for a regular struct with public fields")
Expand Down
33 changes: 31 additions & 2 deletions scroll_derive/tests/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ fn test_data (){
assert_eq!(data.id, 0xdeadbeefu32);
assert_eq!(data.timestamp, 0.5f64);
let mut bytes2 = vec![0; ::std::mem::size_of::<Data>()];
bytes2.pwrite_with(data, 0, LE).unwrap();
bytes2.pwrite_with(&data, 0, LE).unwrap();
let data: Data = bytes.pread_with(0, LE).unwrap();
let data2: Data = bytes2.pread_with(0, LE).unwrap();
assert_eq!(data, data2);
Expand Down Expand Up @@ -83,7 +83,7 @@ fn test_iowrite (){
assert_eq!(bytes_null, bytes);

let mut bytes_null = [0u8; 8];
bytes_null.cwrite_with(data, 0, LE);
bytes_null.cwrite_with(&data, 0, LE);
println!("bytes_null: {:?}", &bytes_null);
println!("bytes : {:?}", &bytes);
assert_eq!(bytes_null, bytes);
Expand Down Expand Up @@ -158,3 +158,32 @@ fn test_nested_struct() {
assert_eq!(read, size);
assert_eq!(b, b2);
}

#[derive(Debug,Copy,Clone,Pwrite,Pread,IOwrite,IOread)]
#[repr(packed)]
struct PackedStruct {
a: u8,
b: u32,
}

#[test]
fn cwrite_packed_struct() {
use scroll::{Cwrite, Cread};
let mut bytes = [0u8; 5];
&bytes[..].cwrite(&PackedStruct{ a: 1, b: 2 }, 0);

let PackedStruct{ a, b } = bytes.cread(0);
assert_eq!(a, 1);
assert_eq!(b, 2);
}

#[test]
fn pwrite_packed_struct() {
use scroll::{Pwrite, Pread};
let mut bytes = [0u8; 5];
&bytes[..].pwrite(&PackedStruct{ a: 1, b: 2 }, 0).unwrap();

let PackedStruct{ a, b } = bytes.pread(0).unwrap();
assert_eq!(a, 1);
assert_eq!(b, 2);
}
Loading