Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the unnecessary braces around From implementations. #66

Open
diseraluca opened this issue May 20, 2021 · 1 comment · May be fixed by #68
Open

Remove the unnecessary braces around From implementations. #66

diseraluca opened this issue May 20, 2021 · 1 comment · May be fixed by #68

Comments

@diseraluca
Copy link

diseraluca commented May 20, 2021

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
Copy link
Author

diseraluca commented May 20, 2021

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.

diseraluca added a commit to diseraluca/uavcan that referenced this issue May 27, 2021
The modular bitfield crate produces an unneccesary braces warning when `From`
implementations are generated.

See: Robbepop/modular-bitfield#66

A pull request ( Robbepop/modular-bitfield#68 ) to
resolve this was provided.

Since it is not yet accepted mainstream, the `modular-bitfield` dependency was
temporarily moved to a custom fork to move forward with the removal of
compilation warnings.
diseraluca added a commit to diseraluca/modular-bitfield that referenced this issue May 28, 2021
Recently, `generate_bitfield_size` was modified to return an expression not
contained in a block, to avoid generating an `unused_braces` warning.
See: Robbepop#66

To simplify the reading of the generated code and avoid ambiguities with
operators' precedence, further parenthesization was added to the expression
itself.

The new parenthesization generates an `unused_parenthesis` warning and is now removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant