dzamlo / rust-bitfield

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

Add from support for the setters #21

Closed roblabla closed 5 years ago

roblabla commented 5 years ago

This PR adds the ability for setters to take a type to convert back into an u32.

u32, from into FooBar, field4, set_field4: 10, 0;

will generate the following setter:

fn set_field4(&mut self, value: FooBar) {
    use bitfield::BitRange;
    self.set_bit_range(10, 0, bitfield::Into::<u32>::into(value));
}

This allows making more uniform APIs to access fields.

dzamlo commented 5 years ago

Thanks a lot for your pull request!

Maybe we should make the setter generic and accept any T: From (or the type of the field), even if we don't specify a keyword. This way all setter will be more flexible. What do think? Do you see any drawbacks to that?

Also, I don't see any tests. I will have time on Monday to add them if you don't do that in the meantime.

roblabla commented 5 years ago

Maybe we should make the setter generic and accept any T: From (or the type of the field), even if we don't specify a keyword.

I don't think that's better. From a documentation perspective, I want the setter to take a specific type so the user can easily access that type and get more documentation about it. As a practical example, here I want to ensure the user will only ever handle instances of DeliveryMode. Its representation as an u32 should be an implementation detail.

And yeah, I didn't write any tests (apart from the doctest I guess). I'll make up some time tomorrow to write them :).

dzamlo commented 5 years ago

Released in 0.13.2

dzamlo commented 5 years ago

I thought of something else. It would be a breaking change, but wouldn't it make sense to set the setter type when there is just the into keyword? Making the from useless.

The where you want into with a setter that use another seems pretty rare. And you can still achieve that with two field.

roblabla commented 5 years ago

Yeah, I didn't want to make a breaking change but having into alone affect the setter seems ideal. If this is going to be tackled, it'd be nice to fix #7 at the same time since it would also require a breaking change.