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

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

Open
andreeaflorescu opened this issue Apr 19, 2022 · 6 comments
Open
Labels
good first issue Good for newcomers

Comments

@andreeaflorescu
Copy link
Member

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 andreeaflorescu added the good first issue Good for newcomers label Apr 19, 2022
@andreeaflorescu
Copy link
Member Author

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
Copy link

uran0sH commented Apr 22, 2022

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
Copy link
Member Author

Why is the Copy trait needed?

@uran0sH
Copy link

uran0sH commented Apr 26, 2022

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
Copy link

uran0sH commented Apr 26, 2022

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
Copy link
Contributor

JonathanWoollett-Light commented May 19, 2022

I think both #36 and #38 cover this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants