m4b / scroll

Scroll - making scrolling through buffers fun since 2016
MIT License
151 stars 36 forks source link

scroll 1.0 #57

Open m4b opened 5 years ago

m4b commented 5 years ago

It is time for scroll to become 1.0

I believe the work started by @willglynn and seconded by others, namely, having pwrite take a reference is a good idea, is highly invasive breakage, and probably top priority item.

I may also swap the default endianness from () back to ctx::Endian, unless someone protests highly?

I would also like to survey unsafe, remove commented code, and some other details.

So:

  1. [ ] Pwrite references
  2. [ ] Default endianness ?
  3. [ ] Remove usize/isize impls
  4. [ ] Add a &str pwrite?

semi-random people who might be interested (feel free to ignore if busy, etc. :):

@luser @philipc @lzutao @willglynn

zicklag commented 4 years ago

What does it mean to have Pwrite take a reference? I've just found this library and I'm trying to understand if it will help solve my use-case. I'm curious what Pwrite references would change.

Edit: Oh, nevermind, I found the PR. :smiley: Sorry for the ping.

m4b commented 4 years ago

Yea so generally it's not a very big deal; i did run into this when i was pwriting very large C structs, and it caused stack overflows, but this is unusual usecase. Similarly, preading out a large C struct is quite difficult without (I believe) very large changes, since it would have to emplace/box the struct without ever touching the stack (otherwise, same issue of stack overflow).

But these are generally niche issues.

I think the only thing on this list that for sure will happen is removing usize/isize; this was just a fundamnetal mistake on my part, and I don't think the crate should support it (you will be forced to think about the bit width size, i.e.)

m4b commented 4 years ago

That being said, I do think pwriting references is generally just a good idea, but not sure if we can find a hero to push it through; there were some final concerns at the end w.r.t. ergonomics, as well, but nothing major i believe.

What usecases are you imagining using scroll for @zicklag, maybe I could be of help answering questions?

luser commented 4 years ago

I've found myself wondering occasionally whether we could make scroll work with serde's trait system, and just implement it in terms of Serializer / Deserializer without making the performance significantly worse. Have you ever given this any thought?

zicklag commented 4 years ago

@m4b As far as my use-case, I was considering as a possible solution to this question of mine on the forum about needing a Vec-like struct that can allow me to index runtime-determined-lengthed byte arrays/slices. This is related to an effort to enable scripting in Shipyard ( https://github.com/leudz/shipyard/issues/96#issuecomment-645654021 ).

I ended up deciding that scroll could be useful for that effort, but we need to get a little further into the next step of the process before I find out exactly what the requirements are for moving on.

m4b commented 4 years ago

@zicklag yup, your question is precisely what scroll was designed for; e.g., one original usecase was reading the Nth elf sym entries out of an array of sizeof::<Sym>() * nentries, for example; it was even designed to pread out in parallel, see for example some integration with rayon there: https://crates.io/crates/lazy_transducer

@luser yea i think about this sometimes, not sure what it would mean or look like? Do you have any ideas on that front? I assume it would be a feature in scroll, or did you mean somewhere else? :)

luser commented 4 years ago

@luser yea i think about this sometimes, not sure what it would mean or look like? Do you have any ideas on that front? I assume it would be a feature in scroll, or did you mean somewhere else? :)

I haven't gone beyond "idle thoughts" with it, but it would be cool if scroll could read and write structs that implemented Serialize / Deserialize instead of having to #[derive(Pread, Pwrite)], since that'd give us a lot more compatibility with other crates. For example, I know we discussed supporting Uuid from the uuid crate in the past, which already has serde support. Obviously you wouldn't want to support the full range of stuff that serde does since there isn't an obvious mapping for all of it to binary formats.

fogti commented 2 years ago

It would be useful to support fallible reading/writing on no_std, which pretty much means returning an error when the buffer is too small / object too big.

Frostie314159 commented 10 months ago

I would suggest removing unsafe code entirely and forbiding it, since it's not actually necessary and safe alternatives are equally fast. I would be happy to author a PR, if an interest exists.

m4b commented 10 months ago

@Frostie314159 if you have some ideas how to remove the unsafe portions without compromising performance, this sounds great! Perhaps a tentative proof of concept PR, or sketch of ideas here if that's less work. My understanding is most unsafe is in the ptr copy non-overlapping writes, which last i checked is also what byteorder does.

Frostie314159 commented 10 months ago

I'm gonna create a tracking issue, where we can discuss and list uses of unsafe.

Frostie314159 commented 10 months ago

We should probably remove the FromCtx impls for numeric types, since we can't uphold our own guarantee, that those impls must not fail.