Pread/Pwrite redefinitions

[nickbclifford]

This PR adds a new type, DynamicBuffer, that automatically expands when Pwriteing types into it. Details in the doc comments.

It also includes a relaxed definition of the Pwrite trait that, while technically an API break, is source-compatible and makes implementing custom containers easier.

Resolves #78.

[nickbclifford]

I was considering adding a TryIntoCtx<_, DynamicBuffer> impl to the Pwrite derive macro as well, but (no offense intended!) the proc macro doesn't seem to be documented well and I didn't really fully understand the specifics of how it worked, so I ultimately decided not to.

[nickbclifford]

Could I get a review @m4b?

[m4b]

also it doesn't look like there's any CI anymore on scroll which is terrifying? (it used to run on travis, but it is probably dead?)

[nickbclifford]

I'd be okay with moving DynamicBuffer into a separate crate, I believe all that would be necessary are the changes for Pwrite.

However, I do agree that the asymmetry with Pread is annoying, so I'll change that as well.

[nickbclifford]

Let me know what you think about the trait changes, and I'd be happy to move the buffer stuff out of this PR and change scope.

[m4b]

I have not forgotten about this! I will try to review tomorrow, thanks for your patience; always feel free to ping me if you aren't hearing from me, also :)

[m4b]

Ok I'm glad I played with this a bit, I do not think this design is implementable anymore:

impl<Ctx: Copy, E: From<error::Error>, const LEN: usize> Pwrite<Ctx, E> for [u8; LEN] {
    fn pwrite_with<N: TryIntoCtx<Ctx, Self, Error = E>>(
        &mut self,
        n: N,
        offset: usize,
        ctx: Ctx,
    ) -> result::Result<usize, E> {
        if offset >= LEN {
            return Err(error::Error::BadOffset(offset).into());
        }
        let dst = &mut self[offset..];
        n.try_into_ctx(dst, ctx)
    }
}

specifically, because TryIntoCtx<Ctx, Dst> has Dst constrained to Self due to the Pwrite trait definition. We could potentially get over this problem by writing something like:

impl<Ctx: Copy, E: From<error::Error>, const LEN: usize> Pwrite<Ctx, E> for [u8; LEN] {
    fn pwrite_with<N: TryIntoCtx<Ctx, Self, Error = E>>(
        &mut self,
        n: N,
        offset: usize,
        ctx: Ctx,
    ) -> result::Result<usize, E> {
        if offset >= LEN {
            return Err(error::Error::BadOffset(offset).into());
        }
        let dst: &mut [u8; LEN] = &mut self[offset..].try_into().unwrap();
        n.try_into_ctx(dst, ctx)
    }
}

But this is incorrect; specifically, we don't want a dst size of LEN, but actually std::mem::size_of::<N>; alas however, const generics don't allow writing std::mem::size_of::<N> in a generic context just yet it seems :(

Nevertheless, even if we could, the definition of Pwrite won't let us I believe, since it will again diverge from the trait definition, since std::mem::size_of::<N> != LEN.

This leads me to believe, ironically, since I suggested to remove it, that the associated type Buffer might have to return, as this allows the implementee to diverge from the TryInto/From arguments. Specifically, I believe it would allow implementing for [u8; LEN] but the byte sync is either &mut [u8] or &mut [u8; size_of::<N>].

It's also possible the original design of scroll would allow this as well, but we would have to implement MeasureWith, etc.

Currently I'm more positive towards your current design as it has the following attractive properties:

1. It simplifies the generic bounds all over (no RangeWithExclusive etc.)
2. It makes it straightforward to implement for different Source buffers (implementees)
3. It appears to drop the use of MeasureWith (which always seemed like an unnecessary hack).

But again, would like to think through various scenarios here, as as we've seen, it's quite tricky to imagine various usecases (I would not have guessed the [u8; LEN] implementation would fail in that manner unless I tried it) but I think we're getting close to the right, semi-future proof design.

Anyway, thanks for being patient here.

[m4b]

Ah, and quite sadly, even with associated type Buffer, we cannot ever write a const generic dst buffer (e.g., if future const generics allow generics in a const position), because the required size, N, is not in scope yet :(

impl<Ctx: Copy, E: From<error::Error>, const LEN: usize> Pwrite<Ctx, E> for [u8; LEN] {
    type Buffer = [u8; std::mem::size_of::<N>()]; // N is not in scope yet...
    fn pwrite_with<N: TryIntoCtx<Ctx, Self::Buffer, Error = E>>(
        &mut self,
        n: N,
        offset: usize,
        ctx: Ctx,
    ) -> result::Result<usize, E> {
        if offset >= LEN {
            return Err(error::Error::BadOffset(offset).into());
        }
        let dst: &mut [u8; std::mem::size_of::<N>()] = &mut self[offset..].try_into().unwrap();
        n.try_into_ctx(dst, ctx)
    }
}

I've just tested, and it is sufficient to write into a slice however, e.g., the following does work:

impl<Ctx: Copy, E: From<error::Error>, const LEN: usize> Pwrite<Ctx, E> for [u8; LEN] {
    type Buffer = [u8];
    fn pwrite_with<N: TryIntoCtx<Ctx, Self::Buffer, Error = E>>(
        &mut self,
        n: N,
        offset: usize,
        ctx: Ctx,
    ) -> result::Result<usize, E> {
        if offset >= LEN {
            return Err(error::Error::BadOffset(offset).into());
        }
        let dst = &mut self[offset..];
        n.try_into_ctx(dst, ctx)
    }
}

[nickbclifford]

I'm not quite sure I understand the usecase behind implementing for an array buffer, can't it already use the [u8] impl from a &mut reference?

[nickbclifford]

If you'd like to discuss this more synchronously outside of GitHub as well, feel free to reach out over email and we can set up messaging or something.

[m4b]

So the idea behind targeting arrays is that the compiler might be able to elide some checks, etc., especially if it sees the type(s) always being smaller than array target, etc. but that could be wishful thinking :shrug:

i think it would be nice to at least have the option to be able to implement arrays, but maybe I could be convinced this isn't useful?

Anyway, so w.r.t. this PR i think ti would be good to remove the dynamic buffer portion so we can concentrate on any remaining issues with the design of the traits, then we can think of merging.

also feel free to email me anytime :)

[nickbclifford]

I think I see your point, but I worry that it might not be sound and/or possible to implement such logic because of types without a constant bytesize. AFAIK, unless we had some sort of marker or auto trait that allowed us to discriminate between those and primitives or PODs, I don't think there'd be a way to write a coherent impl.

Apologies if I didn't explain that well, I just think it might be a premature optimization.

[nickbclifford]

@m4b Any thoughts about merging soon?

[m4b]

@nickbclifford ok, so here's what we should do:

1. if you can squash all your commits to single, and force push, that will make for a nice final review
2. I am inclined to merge this breaking change, without the type Buffer associated type; although this prevents some uses of const buffers, this may not ever matter, it would be an interesting case study to examine the assembly output to see if rustc is able to do anything with array vs slice

Unrelated to your patch, but you probably want a 0.11 crates release at some point:

1. i will likely remove the usize impl for pread and pwrite #56
2. i will possibly look at making pwrite take & references, which is something i wanted for some time, though it may be a huge change, which is related to #51 and was the PR that never got merged here #47

I would really want to revisit pwrite taking & before releasing 0.11, which at this stage would be effectively a beta for scroll 1.0. Before 1.0 i think pwrite taking references is good future proof design.

So in summary, if you can squash your changes, I think i'm ok to merge, and then i'd like to see at least 1. (but probably not 2., i'm not sure who would do it and i don't have time right now) before releasing 0.11

Does that sound reasonable? And once again, thanks for your patience here :)

[nickbclifford]

Yep, sounds all good to me! Thank you very much for being accommodating with my first contribution :grin: