-
Notifications
You must be signed in to change notification settings - Fork 133
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
Fix clippy warnings #130
Fix clippy warnings #130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR!
I'm not sure about two of the lints (trivially_copy_pass_by_ref
and inconsistent_digit_grouping
; see comments below), but otherwise this looks good to me!
@@ -370,7 +370,7 @@ impl<'a, P: PhysToVirt> MapperAllSizes for MappedPageTable<'a, P> { | |||
Err(PageTableWalkError::NotMapped) => return TranslateResult::PageNotMapped, | |||
Err(PageTableWalkError::MappedToHugePage) => { | |||
let frame = PhysFrame::containing_address(p3[addr.p3_index()].addr()); | |||
let offset = addr.as_u64() & 0o_777_777_7777; | |||
let offset = addr.as_u64() & 0o7_777_777_777; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The digits grouping was deliberate as it resembles the page table layout. Each 777
block corresponds to a 9-bit page table index and the 7777
block at the end corresponds to the 12-bit page offset. So I would prefer keeping the digits grouped as is. This also applies to the other instances of this commit.
I think it should be possible to disable this clippy lint for these functions by setting it to allow
.
src/addr.rs
Outdated
@@ -148,31 +148,31 @@ impl VirtAddr { | |||
|
|||
/// Returns the 12-bit page offset of this virtual address. | |||
#[inline] | |||
pub fn page_offset(&self) -> PageOffset { | |||
pub fn page_offset(self) -> PageOffset { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be a breaking change, but I'm not sure. If it is breaking, I don't think that the change is worth it because the potential performance improvement is very small, especially with inlining.
I have reverted the commits you mentioned. |
Looks good to me, thanks! |
No description provided.