Making my suggestions more explicit (based against PR branch)

[kitlith]

Felt like there was something getting lost in translation. Figured that I'd try to make it a little more clear, especially my talk about inverting the design of KnownChunk.

Note that you can replace my InvertedChunk with your code for reading in a loop that you mentioned before, the concept is the same in that KnownChunk (under whatever name it ends up with) just stays around for reading a single chunk of known type.

[kitlith]

since I mentioned maybe generating InvertedChunkID using a macro, let's extend my chunk_ids macro that was previously in a comment on a commit:

macro_rules! chunks {
    ($($id:literal => $variant:ident($target:ty)),* $(,)?) => {
        $(
            impl KnownChunkID for $target {
                const ID: FourCC = FourCC(*$id);
            }
        )*
        #[binrw]
        #[brw(little)]
        #[br(import(id: FourCC))]
        #[derive(Debug, PartialEq, Eq)]
        enum InvertedChunkEnum {
            $(
                #[br(pre_assert(id == <$target as KnownChunkID>::ID))]
                $variant($target),
            )*
            Unknown {
                #[br(calc = id)]
                #[bw(ignore)]
                id: FourCC,
                #[br(parse_with = binrw::helpers::until_eof)]
                raw: Vec<u8>
            }
        }
    }
}

chunks! {
    b"A   " => A(A),
    b"B   " => B(B),
    b"C   " => C(C),
    b"MD5 " => Md5(Md5ChunkData),
}

[briandorsey]

This is lovely. You're right that something was getting lost in translation... I didn't (and don't) understand the interaction of generics and traits well enough to write a working generic version of KnownChunk and fell back on macros.

Since you've gone to the effort of writing it all out in a pull request... just to be explicit: are you OK with merging this as an Apache 2.0 licensed submission? If so, I'll merge this and continue the refactor from there. No worries if that was not the intent.

I really appreciate the time you've put into this. Thank you. It's helping me understand Rust better!

[kitlith]

Since you've gone to the effort of writing it all out in a pull request... just to be explicit: are you OK with merging this as an Apache 2.0 licensed submission? If so, I'll merge this and continue the refactor from there. No worries if that was not the intent.

Yep, I'm fine with that. It'll merge right into your PR's branch when you hit the merge button.

I didn't (and don't) understand the interaction of generics and traits well enough to write a working generic version of KnownChunk and fell back on macros

Ah, that doesn't interact well with my tendency to heavily rely on Generics. So:

• Remember that this is based on what I would do (and therefore biases towards a code style I'm comfortable with, but that you might not be comfortable with), and if it would be better to skew towards something that is easier for you to maintain, that is still an option.
• Feel free to ask me questions regarding my usage of generics here, since you state that you don't currently understand it.

[briandorsey]

Thanks for the super kind response. On generics, I should have been more explicit: after reading your implementation, I understand what's going on well enough to build on that pattern (I think! It's always hard to know our own knowledge boundaries! :) ) but I wasn't able to get there myself when I tried it and ended up with macros instead. I like the generics version better (macros feel like a separate language (or two) embedded). I think I know what my next thing to study/practice is.

Merge incoming.

[briandorsey]

@kitlith, this whole arc... starting with your answer to my question on binrw's repo, to your discussion and comments here, to this PR has really made my week. Thanks for being awesome on the internet.

[briandorsey]

Hi @kitlith Apologies for pinging this a month later! I'd like to re-licence this repo to Apache-2.0 OR MIT to match the wider Rust community norms. Are you OK with your submission also being licensed as MIT?

[kitlith]

@briandorsey I'm fine with this! 👍