modular-bitfield icon indicating copy to clipboard operation
modular-bitfield copied to clipboard

Removes the unused braces warning in the autogenerated From implementations.

Open diseraluca opened this issue 3 years ago • 4 comments

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.


  • Tests, even on a clean download of the repository, seems to be currently broken, at least on my system, with multiple failing testes and a certain amount of panics.

    This means that I was unable to use the tests to avoid regressions or prescribed errors. I cannot, hence, be sure about the correctness of each case.

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 avatar May 27 '21 14:05 diseraluca

HI @Robbepop,

Thank you for taking the time to check this.

I've added your suggestion in 910b17df9adfe7dbfd67e0bc0c37ae1cc2ac06e7.

diseraluca avatar May 28 '21 12:05 diseraluca

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?

Robbepop avatar May 28 '21 12:05 Robbepop

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.

diseraluca avatar May 28 '21 15:05 diseraluca

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.

jumpnbrownweasel avatar Feb 10 '22 00:02 jumpnbrownweasel