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

Implement IntoCtx/TryIntoCtx for references to primitive types. #35

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

luser
Copy link
Contributor

@luser luser commented Sep 13, 2018

This should help in fixing m4b/scroll_derive#15 .

impl<'a> IntoCtx<Endian> for &'a $typ {
#[inline]
fn into_ctx(self, dst: &mut [u8], le: Endian) {
(*self).into_ctx(dst, le)
Copy link
Owner

Choose a reason for hiding this comment

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

Ah i see; so this is sort of a "stopgap" solution, and still won't work for non-copy types.

I was also thinking the other day it would be cool to have a pread_ref and it safely performs a mem::transmute for you and ties the reference to the bytes; this came up recently when attempting to pread a huge object onto the stack and blowing up my stack; what i needed was a reference (and it didn't copy), but had to resort to mem::transmute, and check length, and bla bla; seems like scroll could safely automate some of that drudgery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into something like this a while back before deciding that scroll was the sensible option--I thought that what I really wanted was to be able to just transmute references to structs to avoid copying entirely. The recent changes to make references to fields in packed structs unsafe convinced me otherwise, though. You'd have to ensure that the alignment was correct and for structs they'd have to be packed or contain no padding and it just all seems like a hassle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record:

Ah i see; so this is sort of a "stopgap" solution, and still won't work for non-copy types.

This patch only covers the primitive numeric types in scroll, scroll_derive already generates impls for &'a T as well as T, and the latter calls the former so it should be fine for non-Copy types:
https://github.com/m4b/scroll_derive/blob/d8fe9d0a5a7ea21823ce9fd6206766ae81782e4a/src/lib.rs#L95

Additionally m4b/scroll_derive#17 fixes those impls to always use references so that non-Copy struct fields will be handled properly.

@m4b m4b merged commit afbd412 into m4b:master Sep 14, 2018
willglynn pushed a commit to willglynn/scroll that referenced this pull request Nov 2, 2018
Implement IntoCtx/TryIntoCtx for references to primitive types.
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