feature request: integrating with the `os_units` crate

[toku-sa-n]

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.

[josephlr]

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]

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]

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]

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]

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.