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

paging: page ranges can be generated from Range<u64> #224

Closed
wants to merge 1 commit into from
Closed

paging: page ranges can be generated from Range<u64> #224

wants to merge 1 commit into from

Conversation

mental32
Copy link
Member

I noticed that a fair amount of my paging/physframe logic repeated the pattern of constructing a PageRange | PageRangeInclusive from essentially a Range<u64> and I had a few macros around to deal with the boilerplate of doing this stuff, loosely resembling something like:

let page_range = {
    let start_addr = VirtAddr::new(range.start);
    let end_addr = VirtAddr::new(range.end);

    let start_page = Page::containing_address(start_addr);
    let end_page = Page::containing_address(end_addr);

    Page::range(start_page, end_page)
};

This PR adds From<Range<u64>> impls for the page range variants using the same logic as above.

@mental32
Copy link
Member Author

mental32 commented Dec 30, 2020

note however, that this is more of a convenience feature and I'm not sure how useful it'll be to others (but you can see the pattern appearing in the Heap Allocation post grep for "Creating the page range")

This is a straight forward solution to removing the boilerplate however I'm not convinced its the best (writing this took a couple minutes). A better solution could be adding range constructors for pagerange types or exporting a macro that would resolve to the pattern.

@phil-opp
Copy link
Member

phil-opp commented Jan 3, 2021

Thanks for the PR. I agree that some kind of convenience feature would be nice, but I'm not sure whether this is the right approach for the following reasons:

  • In the long term, we want to migrate away from our custom page/frame range types and use the Step trait and native Rust ranges instead (see Implement core::iter::Step for PhysFrame and Page #212). Thus, we should not add any features that we cannot migrate cleanly. This From implementation is probably not possible with the Step trait.
  • The VirtAddr::new function panics on invalid addresses. Our convenience functions shouldn't discourage people from choosing the safer try_new function instead. So we should at least add a corresponding TryFrom implementation.
  • The Page::containing_address function might not be what you want in all situations either. For example, you might want to use from_start_address instead if the addresses are page aligned (e.g. to catch possible bugs early).

@mental32 mental32 closed this Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants