search

Robbepop / modular-bitfield

Macro to generate bitfields for structs that allow for modular use of enums.
Apache License 2.0
155 stars 40 forks source link

Removes the unused braces warning in the autogenerated From implementations. #68

Open diseraluca opened 3 years ago

diseraluca commented 3 years ago

Resolves #66.

Previously, From implementations may have expanded to code similar to the following:

...
where
    [(); { Size }]:
        ::modular_bitfield::private::IsU32Compatible,
...

Where Size is a constant expression that reduces to a usize. With Size being an expression and the context in which it is used, the braces around it are unneeded, producing a warning.

The braces are inserted in impl::bitfield::expand::generate_bitfield_size, surrounding the returned sum-expression.

To avoid the warning, generate_bitfield_size was modified to return a naked expression.

Places where brackets were needed to avoid an incorrect reading of the expression were modified to add the brackets in place. Similarly, places where the brackets were not needed were simplified.

If this is intended to be accepted, please let me know what I should do meet quality control, be it adding tests or something else.

diseraluca commented 3 years ago

HI @Robbepop,

Thank you for taking the time to check this.

I've added your suggestion in 910b17df9adfe7dbfd67e0bc0c37ae1cc2ac06e7.

Robbepop commented 3 years ago

HI @Robbepop,

Thank you for taking the time to check this.

I've added your suggestion in 910b17d.

Can you also confirm that this still fixes the issues with the warnings?

diseraluca commented 3 years ago

HI @Robbepop, Thank you for taking the time to check this. I've added your suggestion in 910b17d.

Can you also confirm that this still fixes the issues with the warnings?

Sorry I've been sloppy with this as I wasn't at my usual computer and tested only on a small playground.rs.

I've now tested with my library and, indeed, the parenthesis generate a warning. This was, for now, reverted in https://github.com/Robbepop/modular-bitfield/pull/68/commits/b9b81df8cc3586653160e6e9765adfbf97b8c2ef.

A similar case to the one for the from implementation was identified in next_divisible_by_8. The brace were similarly removed. See https://github.com/Robbepop/modular-bitfield/pull/68/commits/d707fe2595728cea3b37873425d1e5c56164c98a. I've checked the 4 places where the expression is used and there should be no problem with parenthesization. I'm not sure if a different format should be tried to ease the reading of the expanded expressions.

Furthermore, testing in another library I identified a case that was broken by the original change, in expand_byte_conversion_impls. This was currently fixed in https://github.com/Robbepop/modular-bitfield/pull/68/commits/735e841bedec40c744de0a7de6e7b28d3d176b8a.

I expect a warning to be generated by the new parenthesization when genenerate_target_or_actual_bitfield_value does not generate a sum but only a single value. I wasn't able to test this, currently, as I'm not sure how to generate that case. If you can provide an example to test with I would be glad to do it.

jumpnbrownweasel commented 2 years ago

I can't find a way to suppress this short of adding #[allow(unused_braces)] for the entire module. Adding it above the struct doesn't work.

There are also unused warnings generated.

warning: associated function is never used: `into_bytes`
warning: associated function is never used: `from_bytes`

For this I had to add #[allow(dead_code)] for the entire module.

For now I'll use the bitfield crate instead. I would be happy to use this instead if it looks like it is being actively maintained. I think the API is quite good.