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

Remove the unnecessary braces around From implementations. #66

Open diseraluca opened 3 years ago

diseraluca commented 3 years ago

I'm currently using modular-bitfield for a project and am incurring in warnings generated by the the crate.

When a bitfield structure is annotated with a repr representation, the generated code will implement From-conversions to and from the represented type.

For example:

#[bitfield]
#[repr(u32)]
struct Foo {
    bar: B32,
}

Would produce the following code:

[...]
impl ::core::convert::From<::core::primitive::u32> for Foo
where
    [(); { 0usize + <B32 as ::modular_bitfield::Specifier>::BITS }]:
        ::modular_bitfield::private::IsU32Compatible,
{
    #[inline]
    fn from(__bf_prim: ::core::primitive::u32) -> Self {
        Self {
            bytes: <::core::primitive::u32>::to_le_bytes(__bf_prim),
        }
    }
}
impl ::core::convert::From<Foo> for ::core::primitive::u32
where
    [(); { 0usize + <B32 as ::modular_bitfield::Specifier>::BITS }]:
        ::modular_bitfield::private::IsU32Compatible,
{
    #[inline]
    fn from(__bf_bitfield: Foo) -> Self {
        <Self>::from_le_bytes(__bf_bitfield.bytes)
    }
}
[...]

In the where clause of the From implementations, the constant expression that gives the length of the of the array is enclosed in braces which are unneeded, producing the following compiler warning:

warning: unnecessary braces around const expression
[...]
N  | #[repr(u32)]
     | ^ help: remove these braces
[...]

This From expansion seems to happen in impl::bitfield::expand::expand_repr_from_impls_and_checks, where the following lines expand to the warned-about code:

fn expand_repr_from_impls_and_checks(&self, config: &Config) -> Option<TokenStream2> {
[...]
        let actual_bits = self.generate_target_or_actual_bitfield_size(config);
[...]
        quote_spanned!(span=>
            impl ::core::convert::From<#prim> for #ident
            where
                [(); #actual_bits]: ::modular_bitfield::private::#trait_check_ident,
[...]

Then, in generate_target_or_actual_bitfield_size, if the configuration has a None value for the bits field, the generation is delegated to generate_bitfield_size which generates the expression with the unneeded braces:

fn generate_bitfield_size(&self) -> TokenStream2 {
[...]
    quote_spanned!(span=>
        { #sum }
    )
}

I consider that the correct behavior here would be to avoid generating a warning by removing the braces when there is no need for them.

If it is desirable for the project to have this change, I would gladly make it myself as I need it for my project.

diseraluca commented 3 years ago

Some possible solutions might be:

  1. Add an #[allow(unused_braces)] attribute to the generated implementations.
  2. Remove the block and quote only the expression in expand_repr_from_impls_and_checks.
  3. Remove the block directly in generate_bitfield_size, and reinstate the block locally where it is required.

Solution 3 is what I would consider, albeit it requires knowledge of where blocks are needed and might require some code to discern between an actual sum and a value.

For example, if 3 was to be implemented sloppily, the following code would not work anymore:

#[bitfield]
struct Foo {
   bar: B16,
   baz: B16,
}

As the code generated in impl::bitfield::expand::generate_filled_check_for_aligned_bits would be incorrectly parenthesized.

This can easily be solved locally by parenthesizing the expression itself, knowing that a 0-fields bitfield cannot exist and that the format of generate_bitfield_size has, hence, always at least 2 addends, as follows:

fn generate_filled_check_for_aligned_bits(&self, config: &Config) -> TokenStream2 {
[...]
    type Size = ::modular_bitfield::private::checks::TotalSize<[(); (#actual_bits) % 8usize]>;
[...]

Similar cases may exist but they should be findable by tracing the uses of generate_bitfield_size.