Description
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 commentedon Aug 20, 2021
I don't think we would really need the
Bytes
type. For example, when subtracting twoVirtAddrs
, it's fairly clear that the returnedu64
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 theAdd
/Sub
implementations forPage<S>
andPhysFrame<S>
?EDIT: If the arithmetic ops are the only reason to have a
NumPages
type, it might make more sense to just not have aSub
impl forPage
, and just explicitly havebytes_between
/pages_between
methods.toku-sa-n commentedon Aug 20, 2021
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 commentedon Aug 20, 2021
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 thePage<S>: Sub<Self>
implementation. However, if we did that, would we still want to keep thePage<S>: Add<u64>
andPage<S>: Sub<u64>
implementations? It seem like we should either allow all of:or none of it. All three implementations run the risk of ambiguity between a number of bytes and a number of pages.
phil-opp commentedon Aug 21, 2021
Some early thoughts:
I agree that
Add<u64>
/Sub<u64>
andSub<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
andbytes_between
. It sounds like it returns the absolute value of the difference, i.e. always a positive value. So users might assume thatx.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 someBytes
andNumberOfPages
wrappers so that we can still implementAdd
andSub
(otherwise we would have to add explicitadd_bytes
/add_pages
methods). One potential problem with that could be that the meaning ofNumberOfPages
kind of differs depending on the page size of the page you add it too.josephlr commentedon Aug 22, 2021
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 forPage
andPhysFrame
, it seems nice that adding1
corresponds to a step in a range.CheckedAdd
andCheckedSub
traits #293