dzamlo / rust-bitfield

This crate provides macros to generate bitfield-like struct.
Apache License 2.0
157 stars 19 forks source link

Add an option to opt-out from default `BitRange` implementation on "newtype" #3

Closed ithinuel closed 6 years ago

ithinuel commented 6 years ago

The only current way to have a custom implementation of BitRange for a given type is to invoke all bitfield_debug, bitfield_fields...

This PR adds the ability to opt-out from the default BitRange implementation provided by bitfield! by adding no default BitRange right after the type definition.

This only applies to the 'newtype' pattern.

dzamlo commented 6 years ago

Thanks for this PR!

I'v made some change that I pushed to you branch.

I have a few comments:

I'm not sure if no default BitRange is the best syntax for that. What do you think of impl !BitRange? This mimics the syntax to opt-out of Sync and Send.

If you use that and don't implement BitRange<u8>, you don't get the default implementation of Bit and thus can't use the single bit syntax, unless you implement Bit yourself. I'm not sure this is clear enough in the documentation.

ithinuel commented 6 years ago

Hi !

Indeed, I wasn't sure about the syntax. What made me choose this one is because impl !BitRange looks too much like forbit BitRange for. impl !Sync does actually conflicts with impl Sync.

It took me sometime to understand why when I was using BitRange<u32> I couldn't get Bit for free too. I tried to change this implementation to make it compatible with all numeric types by using the trait bound Into<T> + From<T> but they doesn't actually exist for bool and it forces us to use casting instead. So maybe adding some emphasis on that point in the documentation could be a good thing too.

Anyway, Thank you very much for the great work !

dzamlo commented 6 years ago

The way I implemented was to keep the responsibility of generating the structure in bitfield_struct and avoid repeating ourselves.

My issue with you approach is that we expose this functionality in bitfield_struct, and I don't think this really makes senses.

Indeed, I wasn't sure about the syntax. What made me choose this one is because impl !BitRange looks too much like forbit BitRange for.

I was reading this syntax more as "disable the automatic implementation". But I think your point is correct. I'm not a fan of the chosen syntax, but I can't find anything better, so I will keep no default BitRange

... I couldn't get Bit for free too. I tried to change this implementation to make it compatible with all numeric types ...

I couldn't find a way to implement it either. Either there was a missing implementation is std or the coherence rules were incompatible with my implementation.

dzamlo commented 6 years ago

I will merge this as is and publish a new version shortly.

Thanks again for your PR !!! :smile:

ithinuel commented 6 years ago

Great !

I'm still working on few things in this crate as I'll be using it intensively in https://github.com/ithinuel/rusty-printer

especially #7

ithinuel commented 6 years ago

My issue with you approach is that we expose this functionality in bitfield_struct, and I don't think this really makes senses.

I think I know what you mean indeed. What about moving all newtype generations to bitfield! and change bitfield_struct! to something like bitfield_bitrange! making it responsible of generating only default implementations ?

Then we'd have : bitfield! :

bitfield_debug :

bitfield_bitrange :

bitfield_fields :

This would at the same time simplify bitfield_bitrange.

dzamlo commented 6 years ago

I thinks this would make a lot of sense, especially with the fact that the bitfield_bitrange interface could be really simplified.