search

wojciech-graj / bin-proto

Simple bit-level protocol definitions in Rust.
MIT License
17 stars 1 forks source link

Discriminant and repr #1

Closed vhdirk closed 5 months ago

vhdirk commented 5 months ago

I'm looking into bin-proto as an alternative to deku (since I like performance :) ) for a rather complex binary protocol: https://github.com/vhdirk/dash7-rs

One thing that kind of bugs me about deku, and is also present in bin-proto, is the need to specify the discriminant type and value using annotations, instead of Rust's built in explicit discriminant definitions, combined with repr(uX).

So I am curious why annotations are needed at all? And if you would be willing to accept contributions that make using a 'native' discriminant possible. For one thing, it would make my life a little easier when creating bindings.

wojciech-graj commented 5 months ago

My only reason for picking to use a custom annotation instead of repr(uX) was because I had no clue this was an actual rust feature, but it's exactly what's needed here.

And I'm always happy to receive contributions, but I'm planning to rewrite the derive macro codegen this weekend, so I'll just make this change as well.

vhdirk commented 5 months ago

Oh cool, in that case I am looking forward to your updates. Do you plan on adding support for padding before/after? And passing a ctx like in deku?

wojciech-graj commented 5 months ago

Context passing is currently possible, but it's implemented in a very C way, in that you just get a &mut Any, and you can cast it to whatever type you're actually using. I would like to rework this to make the context a generic that you specify when you impl Protocol, but that will require the improved codegen, so you can expect this quite soon. And padding is quite low on my priority list as it's not needed for my personal use-case, so it might take a while, but if it'll be the only thing stopping you from using bin-proto, you could consider opening a PR with this functionality.

wojciech-graj commented 5 months ago

After doing some research and experimentation, I've decided to stick with the current approach for discriminants. Unfortunately std::mem::discriminant returns a std::mem::Discriminant, which cannot be converted to a primitive without relying on UB. According to this forum post, the only "correct" way to get the numeric value of a discriminant is by using core::intrinsics::discriminant_value, but that's not possible until that is stabilized. Also, a downside of using repr(X) is that it forces the compiler's hand in terms of how it lays out the struct in memory, and requires the value of the discriminant to be prepended to this representation, which will almost definitely not improve performance, but has the potential to impact it negatively.

As for context handling, I fully reworked it, and would argue that the system currently used by bin-proto is pretty much as good as it gets, and is (in my personal opinion) much more comfortable to use than Deku's. Check out the sections on ctx and ctx_bounds in the docs, as well as the ctx tests.

I'm planning to tackle bit/byte alignment in version 0.5, which should release within the next few days if all goes well.

I'll close this issue, but feel free to leave another comment or open another issue if you have any other suggestions for functionality, or any additional remarks :))