Skip to content

Generate safe bits writers when possible#554

Merged
bors[bot] merged 1 commit into
rust-embedded:masterfrom
newAM:safe-bits-writer
Dec 4, 2021
Merged

Generate safe bits writers when possible#554
bors[bot] merged 1 commit into
rust-embedded:masterfrom
newAM:safe-bits-writer

Conversation

@newAM

@newAM newAM commented Dec 4, 2021

Copy link
Copy Markdown
Member

The 'bits' method of register writers can be safe if:

  • there is a single field that covers the entire register
  • that field can represent all values

@newAM
newAM requested a review from a team as a code owner December 4, 2021 18:59
@rust-highfive

Copy link
Copy Markdown

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @adamgreig (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Dec 4, 2021
@burrbull

burrbull commented Dec 4, 2021

Copy link
Copy Markdown
Member

map(fn).flatten() could be replaced with flat_map(fn)

Explicit deref also unneeded

@newAM

newAM commented Dec 4, 2021

Copy link
Copy Markdown
Member Author

map(fn).flatten() could be replaced with flat_map(fn)

I think flat_map is only an Iterator method: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flat_map

The map/flatten chains are on single Option<T> types to access deeply nested data, I couldn't find a flat_map method for Option<T>.

Explicit deref also unneeded

Done! Good catch.

@burrbull

burrbull commented Dec 4, 2021

Copy link
Copy Markdown
Member

I think flat_map is only an Iterator method: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flat_map

I missed this is Option. What about and_then?

adamgreig
adamgreig previously approved these changes Dec 4, 2021

@adamgreig adamgreig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @burrbull ?

@burrbull

burrbull commented Dec 4, 2021

Copy link
Copy Markdown
Member

LGTM, @burrbull ?

This will not compile.

@burrbull

burrbull commented Dec 4, 2021

Copy link
Copy Markdown
Member

LGTM, @burrbull ?

This will not compile.

if register
            .fields
            .as_ref()
            .and_then(|fields| fields.iter().next())
            .and_then(|field| field.write_constraint)
            .and_then(|constraint| match constraint {
                WriteConstraint::Range(range)
                    if range.min == 0 && range.max == 2_u64.pow(rsize) - 1 =>
                {
                    None
                }
                _ => Some(()),
            })
            .is_some()
        {
            mod_items.extend(quote! {
                #[doc = "Writes raw bits to the register."]
                #[inline(always)]
                pub unsafe fn bits(&mut self, bits: #rty) -> &mut Self {
                    self.0.bits(bits);
                    self
                }
            });
        } else {
            mod_items.extend(quote! {
                #[doc = "Writes raw bits to the register."]
                #[inline(always)]
                pub fn bits(&mut self, bits: #rty) -> &mut Self {
                    unsafe { self.0.bits(bits); }
                    self
                }
            });
        }

@thejpster

Copy link
Copy Markdown
Contributor

I've looked at this twice now and I can't for the life of me work out what's going on here.

If it's just me, then that's ok. But if anyone else is struggling, maybe some comments would be useful, or to break up the big chain into smaller pieces.

But I love the theory, and this will clean up a bunch of code.

@newAM

newAM commented Dec 4, 2021

Copy link
Copy Markdown
Member Author

LGTM, @burrbull ?

This will not compile.

Can you explain more?

I am testing this with the STM32WL and stm32-rs, I can compile the generated crate with these changes.

Edit: Nevermind, you're right. Something broke.

I missed this is Option. What about and_then?

Good catch, that's much better. Changed.

@burrbull

burrbull commented Dec 4, 2021

Copy link
Copy Markdown
Member

Can you explain more?

self.0.bits(bits) is unsafe

@newAM

newAM commented Dec 4, 2021

Copy link
Copy Markdown
Member Author

Can you explain more?

self.0.bits(bits) is unsafe

Fixed! I forgot a set -e in my bash script that I used for checking this :(

I've looked at this twice now and I can't for the life of me work out what's going on here.

If it's just me, then that's ok. But if anyone else is struggling, maybe some comments would be useful, or to break up the big chain into smaller pieces.

I broke it up to separate the logic of "can the write be safe" and the code generated as a result. Let me know if that helped :)

Comment thread src/generate/register.rs Outdated
The 'bits' method of register writers can be safe if:
* there is a single field that covers the entire register
* that field can represent all values
@burrbull

burrbull commented Dec 4, 2021

Copy link
Copy Markdown
Member

bors r+

@bors

bors Bot commented Dec 4, 2021

Copy link
Copy Markdown
Contributor

Build succeeded:

@bors
bors Bot merged commit b45bdce into rust-embedded:master Dec 4, 2021
@newAM
newAM deleted the safe-bits-writer branch December 4, 2021 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants