rust-vmm / vm-allocator

Provides allocations and release strategies for resources used during the lifetime of a VM.
Apache License 2.0
30 stars 24 forks source link

Use std::ops::Range instead of reimplementing the functionality #23

Open andreeaflorescu opened 2 years ago

andreeaflorescu commented 2 years ago

The Range object already exists in Rust. Instead of reimplementing the functionality, we can just extend such that we can add the needed functionality, which for now is just theoverlaps function.

Also, depending on how the crate is shaped, we will need to use either Range or RangeInclusive, where the latter represents a range that is inclusive on both ends:

andreeaflorescu commented 2 years ago

To make it easy to use, the crate defined range should define Deref and DerefMut such that all functions defined in std::ops are also available for the crate defined range. This is following the NewType Rust pattern: https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#using-the-newtype-pattern-to-implement-external-traits-on-external-types

uran0sH commented 2 years ago

If we use std::ops::RangeInclusive, we also need to implement PartialOrd, PartialEq for RangeInclusive. And std::ops::RangeInclusive doesn't implement Copy trait

andreeaflorescu commented 2 years ago

Why is the Copy trait needed?

uran0sH commented 2 years ago

The Copy trait doesn't seem to be necessary. std::ops::RangeInclusive.contain() is to check one number in this range. So we also implement the contain function. This means we only need len, start, end function in std::ops::RangeInclusive

uran0sH commented 2 years ago

The sample code:

#[derive(Clone, Debug)]
pub struct RangeInclusive {
    inner: std::ops::RangeInclusive<u64>
}

impl PartialEq for RangeInclusive {
    fn eq(&self, other: &Self) -> bool {
    }
}

impl Eq for RangeInclusive {}

impl PartialOrd for RangeInclusive {
    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
    }
}

impl Ord for RangeInclusive {
    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
    }
}

impl RangeInclusive {
    pub fn new(start: u64, end: u64) -> Result<Self> {

    }

    pub fn contains(&self, other: &RangeInclusive) -> bool {

    }

    pub fn overlaps(&self, other: &RangeInclusive) -> bool {

    }
}
JonathanWoollett-Light commented 2 years ago

I think both https://github.com/rust-vmm/vm-allocator/pull/36 and https://github.com/rust-vmm/vm-allocator/pull/38 cover this.