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 reborrows? #1973

Merged
merged 1 commit into from Sep 20, 2020
Merged

Remove reborrows? #1973

merged 1 commit into from Sep 20, 2020

Conversation

ppannuto
Copy link
Member

Pull Request Overview

We use a re-borrow idiom everywhere we access registers. Once upon a lifetime, when MMIO looked very different, I think this was necessary as we changed types to something that let us access the hardware. After the introduction of StaticRef, however, I don't think we actually need this anymore and we can simplify all of the register accesses.

There ~300 more instances of this &* register pattern in the kernel, so I wanted to take a temperature check on this before doing more of them.

Testing Strategy

Compiling.

TODO or Help Wanted

Is there are a reason to keep all of these &* around?

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

Comment on lines -367 to -373
let regs = &*self.registers;

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't need this line of code with the obscure &* everywhere, then I definitely want it gone.

@brghena
Copy link
Contributor

brghena commented Jun 22, 2020

This could also be a good time to standardize whether the reference to registers is called regs or registers. Your two example files make different choices.

@bradjc
Copy link
Contributor

bradjc commented Jun 22, 2020

I support this.

This could also be a good time to standardize whether the reference to registers is called regs or registers. Your two example files make different choices.

I don't, however, think we need to manage variable names at this level.

@phil-levis
Copy link
Contributor

I support this.

This could also be a good time to standardize whether the reference to registers is called regs or registers. Your two example files make different choices.

I don't, however, think we need to manage variable names at this level.

I don't think we need to manage them, but consistency is good. I.e., we're not going to reject a PR because it says regs instead of registers, but there is no good reason to have variation. However, if you have consistency, then new contributors will see it and follow it.

@ppannuto ppannuto added the release-blocker Issue or PR that must be resolved before the next release label Sep 18, 2020
alevy
alevy previously approved these changes Sep 18, 2020
These were a holderover from older MMIO strategy.

Pseudo-RFP in original draft of #1973; discussed on call 2020-09-18.
@ppannuto
Copy link
Member Author

I think I got all of the ones that make sense to do as part of a mass refactor. We sill have the &* idiom in a few places, but most of those seem necessary.

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

Too much for me to look through...hopefully it is all good :)

@phil-levis
Copy link
Contributor

bors r+

@ppannuto
Copy link
Member Author

bors retry

@bors bors bot merged commit 024738f into master Sep 20, 2020
@bors bors bot deleted the no-reborrow branch September 20, 2020 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Issue or PR that must be resolved before the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants