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

scroll 1.0 #57

Open
4 tasks
m4b opened this issue Sep 6, 2019 · 12 comments
Open
4 tasks

scroll 1.0 #57

m4b opened this issue Sep 6, 2019 · 12 comments

Comments

@m4b
Copy link
Owner

m4b commented Sep 6, 2019

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
Copy link

zicklag commented Jun 28, 2020

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. 😃 Sorry for the ping.

@m4b
Copy link
Owner Author

m4b commented Jun 30, 2020

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
Copy link
Owner Author

m4b commented Jun 30, 2020

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
Copy link
Contributor

luser commented Jun 30, 2020

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
Copy link

zicklag commented Jun 30, 2020

@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 ( leudz/shipyard#96 (comment) ).

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
Copy link
Owner Author

m4b commented Jul 1, 2020

@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
Copy link
Contributor

luser commented Jul 1, 2020

@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
Copy link

fogti commented Aug 31, 2022

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
Copy link
Contributor

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
Copy link
Owner Author

m4b commented Nov 5, 2023

@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
Copy link
Contributor

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

@Frostie314159
Copy link
Contributor

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

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

No branches or pull requests

5 participants