Skip to content

feature request: integrating with the os_units crate #288

Open
@toku-sa-n

Description

@toku-sa-n
Member

I'd like to suggest integrating my os_units crate into the x86_64 crate.

Descriptions

The os_units crate provides two types: Bytes and NumOfPages. These types are interactive because of two methods: Bytes::as_num_of_pages and NumOfPages::as_bytes. Note that Bytes::as_num_of_pages returns the number of pages that is more than the bytes the instance of Bytes contains.

Advantages

Some methods will have more explicit return types. For example, Page implements Sub<Self>. However, the return type is u64, and it doesn't provide the information whether the number represents the bytes of pages or the number of pages (at first, I thought it was former). The users can read the docs (by the way, there is no information of its return type except the source code), but returning NumOfPages will prevent bugs because of the mismatch of types.

Drawbacks

The users will have to call as_usize (or as_u64 to match with the address types) each time, increasing the size of the code.

Notes

These two types use usize, not u64. However, changing the type to u64 will be better because this crate mainly uses it.

Activity

josephlr

josephlr commented on Aug 20, 2021

@josephlr
Contributor

I don't think we would really need the Bytes type. For example, when subtracting two VirtAddrs, it's fairly clear that the returned u64 is a number of bytes. I don't know if there is some other reason to have it.

However, having a NumPages<S: PageSize> type is more interesting. Where else would this be used other than in the Add/Sub implementations for Page<S> and PhysFrame<S>?

EDIT: If the arithmetic ops are the only reason to have a NumPages type, it might make more sense to just not have a Sub impl for Page, and just explicitly have bytes_between/pages_between methods.

toku-sa-n

toku-sa-n commented on Aug 20, 2021

@toku-sa-n
MemberAuthor

EDIT: If the arithmetic ops are the only reason to have a NumPages type, it might make more sense to just not have a Sub impl for Page, and just explicitly have bytes_between/pages_between methods.

I think this is a great improvement. Actually, I couldn't find any methods that can use the NumPages type. Can I send a PR?

josephlr

josephlr commented on Aug 20, 2021

@josephlr
Contributor

EDIT: If the arithmetic ops are the only reason to have a NumPages type, it might make more sense to just not have a Sub impl for Page, and just explicitly have bytes_between/pages_between methods.

I think this is a great improvement. Actually, I couldn't find any methods that can use the NumPages type. Can I send a PR?

I would want to get @phil-opp's input from a design perspective. If we added methods like pages_between/bytes_between, then it would make sense to remove the Page<S>: Sub<Self> implementation. However, if we did that, would we still want to keep the Page<S>: Add<u64> and Page<S>: Sub<u64> implementations? It seem like we should either allow all of:

let page1 = Page::containing_address(...);
let page2 = Page::containing_address(...);
let diff: u64 = page2 - page1; // Page<S>: Sub<Self> (would be replaced by pages_between)
let next_page = page1 + 1; // Page<S>: Add<u64>
let prev_page = page2 - 1; // Page<S>: Sub<u64>

or none of it. All three implementations run the risk of ambiguity between a number of bytes and a number of pages.

phil-opp

phil-opp commented on Aug 21, 2021

@phil-opp
Member

Some early thoughts:

I agree that Add<u64>/Sub<u64> and Sub<Self> methods are connected and that we should either keep all or none of them.

I'm not sure if I like the naming of pages_between and bytes_between. It sounds like it returns the absolute value of the difference, i.e. always a positive value. So users might assume that x.pages_between(y) == y.pages_between(x) in all cases, which is probably not how we plan to implement it.

Also, if we go with bytes/pages_between, we should probaby also implement some Bytes and NumberOfPages wrappers so that we can still implement Add and Sub (otherwise we would have to add explicit add_bytes/add_pages methods). One potential problem with that could be that the meaning of NumberOfPages kind of differs depending on the page size of the page you add it too.

josephlr

josephlr commented on Aug 22, 2021

@josephlr
Contributor

I agree with @phil-opp's comments above. Personally I don't find the existing implementations that confusing. Especially if we add a Step implementation for Page and PhysFrame, it seems nice that adding 1 corresponds to a step in a range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @phil-opp@josephlr@toku-sa-n

        Issue actions

          feature request: integrating with the `os_units` crate · Issue #288 · rust-osdev/x86_64