Skip to content

Add core friendly, no_std, no result, Read/Write trait and/or api #7

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

Closed
m4b opened this issue Jun 6, 2017 · 9 comments
Closed

Add core friendly, no_std, no result, Read/Write trait and/or api #7

m4b opened this issue Jun 6, 2017 · 9 comments

Comments

@m4b
Copy link
Owner

m4b commented Jun 6, 2017

Example

Current working draft is on a branch, core_api, and basically lets you do this:

#[repr(packed)]
struct Bar {
    foo: i32,
    bar: u32,
}

impl scroll::ctx::FromCtx for Bar {
    fn from_ctx(bytes: &[u8], ctx: scroll::Endian) -> Self {
        use scroll::Cread;
        Bar { foo: bytes.cread_with(0, ctx), bar: bytes.cread_with(4, ctx) }
    }
}

#[test]
fn cread_api() {
    use scroll::Cread;
    let bytes = [0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0xef,0xbe,0x00,0x00,];
    let foo = bytes.cread::<usize>(0);
    let bar = bytes.cread::<u32>(8);
    assert_eq!(foo, 1);
    assert_eq!(bar, 0xbeef);
}

#[test]
fn cread_api_customtype() {
    use scroll::Cread;
    let bytes = [0xff, 0xff, 0xff, 0xff, 0xef,0xbe,0xad,0xde,];
    let bar = bytes.cread::<Bar>(0);
    assert_eq!(bar.foo, -1);
    assert_eq!(bar.bar, 0xdeadbeef);
}

All the basic numeric types are supported, and anything that implements FromCtx (which is much easier to understand and implement than it's evil cousin, TryFromCtx), will automatically be deseriablizable without result oriented api via:

let your_type = bytes.cread::<YourType>(offset)

as show above in the example.

Yay!

Some considerations

  1. Need to decide on name
  2. Need to decide whether this should be incorporated in another trait like pread, and add extra methods there (and what those methods should be called)

I'm partial to 2., since it doesn't require adding another trait as an import; but then again, perhaps who cares?

Of the maybe 1 or 2 people paying attention, any opinions welcome. 😎

@m4b m4b added this to the 0.7.0 milestone Jun 6, 2017
@m4b m4b self-assigned this Jun 6, 2017
@andylokandy
Copy link
Collaborator

andylokandy commented Jun 6, 2017

What is the difference between trait pread and cread? They look the same to me.

@m4b
Copy link
Owner Author

m4b commented Jun 6, 2017

The short story:

Pread returns Result<T>, for bytes.pread::<T>(offset)

Cread returns T for bytes.cread::<T>(offset)

The long story:

Pread uses the conversion trait TryFromCtx which is much more complicated, has an Error associated type (similar to the experimental TryFrom being considered for inclusion in std), and even allows lifetimes. We can pread out &str safely, etc.

Cread uses the conversion trait FromCtx, which cannot fail (similar to standard From trait), and doesn't have lifetimes. The idea is it's for basic types, and also now used in lread to allow simple allocationless, std Read stream based API.

If you peak in the file ctx.rs, you'll see every numeric type implements TryFromCtx by actually calling it's FromCtx impl (logically, anything that implements FromCtx, can implement TryFromCtx trivially, and is also being discussed on the TryFrom std thread/RFC)

TryFromCtx is also additionally complicated because I foolishly pushed the offset logic into the trait impl, and used a (usize, Ctx) tuple as the ctx to implement the offsetting. This was a good idea at the time, but also needlessly complicated and stupid on hindsight 🤓 .

I realize now that I can move the indexing logic into the Pread trait itself via bounds, assume for TryFromCtx the deserialization begins at the beginning of the object, like I just did with Cread, and move all bad offset logic into the trait itself, but that is perhaps something for another time.

@andylokandy
Copy link
Collaborator

andylokandy commented Jun 6, 2017

Oh yeah, gotcha.
I've read the source code roughly, and I was wondering why FromCtx trait exist. Because the trait pread doesn't require a FromCtx but TryFromCtx. FromCtx now makes sence sincecread appears.


Something want to say

Awesome Work !!!
I am a developer on embedded system, so I am used to manipulate IO data by pointer in C. When I come to Rust, I want to abstract the process of manipulation into some form of trait. I stopped my work since I found your crate because its API is exactly what I wrote on my draft paper!! And even more generic. Appreciate your work!!

I can tell this crate is so generic to work in many domain, especially in low level networking and hardware communication, while for byte parsing job, the crate byteorder is too light and the crate nom is too heavy.

You could advertise your work to let other's know. AFAIK, here and here are the right place.

Anyway, I can see there are rough edges on API designs.

  • The method pread should not check the bound twice.
    For now the implementation of TryFromCtx checks slice bound for the first time, then call FromCtx which assert the bound for the second time.

  • Trait SizeWith may read into and destruct the data to figure out its size while the method gread() will do it again later. It's especially a performance waste when parsing with a big struct. Maybe we can move the sizeof semantic into TryFromCtx, which turn to return a optional size by Option<Unit> and its data T.

@m4b
Copy link
Owner Author

m4b commented Jun 6, 2017

@goandylok Thanks for the kind words, that's very nice of you to say, and glad it could help :)

And wow seems like you've definitely investigated the source.

So the assert's went into FromCtx because the implementation uses unsafety to get some good performance primarily via a memcpy; but without the asserts, I can send in a buffer smaller than the requested size, and who knows what will happen. Also they weren't there before so anyone sending a small buffer into my numeric impls of FromCtx would get UB 😭

I think it is fixable in TryFromCtx; at the very least could duplicate the impl via some macro, since parsing numbers should definitely be worth that kind of optimization, but might need some thought about best and most maintainable way.

For second point, i'm not quite sure I understand. The SizeWith is basically just size_of analog but with a parsing context, so it should be near constant. However, after realizing I can accomplish more with generic indexing, I think some of the ugliness of the Gread impl might go away (and might even eliminate the need for pread_unsafe).

If you have any suggestions or issues, please feel free to raise and issue and we can work on it :)

@andylokandy
Copy link
Collaborator

andylokandy commented Jun 6, 2017

I have two solutions for the first topic:

  • Mark FromCtx and cread‘s members unsafe to give away the responsibility of bound check to call site.
  • Make FromCtx to be a wrapper of TryFromCtx by calling expect() on TryFromCtx's result.

I will choose to use pread on struct because bound check is important since we usually deal with incomplete data and an incomplete struct doesn't make sence. And I will use cread on primitives because we always do bound check before start parsing struct's fields, so we can easily cread a primitive with confident without bound check. In this situation, the unsafe solution seems better.

For the second topic, maybe I misunderstood gread's semantic. Let's assume that we have a struct, whose memory size is 4 bytes(single field of &'a str) and it preads 20 bytes from a byte slice (where the &'a str points to). I wonder how much will gread increase the offset? If I don't get it wrong, the offset will be increased by 20, which means SizeWith will return 20, and further, SizeWith isn't basically just size_of. Instead, it can compute the underlying data, which may be a header pointing out how long the string buffer is.

@m4b
Copy link
Owner Author

m4b commented Jun 6, 2017

I typed up an enormous response on my phone and then githubrefreshed somehow and deleted everything so that's cool.

The short response is that SizeWith doesn't take self, and Gread is totally generic and requires knowing the size of the type beforehand to parse and increment the offset or return an error.

SizeWith is intentionally not implemented for &str currently (or Uleb128) to force users to think about how to automatically update the offset when variable length data is present in the bytes, which usually requires specifying the size in the context, or just giving up and using pread for the slices and incrementing the offset manually.

I did consider adding a MeasureWith trait that does take self and adding a read that does this for you, but need to think about it

@m4b
Copy link
Owner Author

m4b commented Jun 8, 2017

This is on master now; I actually think I may release new crate version as is soon; but i may want to get #10 done before that, not sure, maybe wait until after, since it will potentially be a big change

@m4b
Copy link
Owner Author

m4b commented Jun 8, 2017

So I think this is done actually, and I think I want to push out a crate for 0.7.0 exposing the new api. I wish it was easier to test no_std setups :/

@m4b
Copy link
Owner Author

m4b commented Aug 8, 2017

This is done

@m4b m4b closed this as completed Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants