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

std: Add missing stability on Range #23434

Merged
merged 1 commit into from
Mar 25, 2015
Merged

Conversation

alexcrichton
Copy link
Member

Now that we check the stability of fields, the fields of this struct should also
be stable.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

Now that we check the stability of fields, the fields of this struct should also
be stable.
@aturon
Copy link
Member

aturon commented Mar 18, 2015

@bors: r+ 5749a1f

@Stebalien
Copy link
Contributor

My two cents: I'd like to hold off on this till we figure out some way to represent other range variants (see my closed RFC: rust-lang/rfcs#952).

@alexcrichton
Copy link
Member Author

Hm I was under the impression that if we added more ranges we would create new types instead of using enums, but I'm also not sure how heavily we're going to be leaning on the syntax to create this wide array of types (which can explode quickly). The in-memory representation of these ranges can also be quite important for both performance and memory consumption. Thoughts though, @aturon?

@bors: r-

@bluss
Copy link
Member

bluss commented Mar 18, 2015

I view them as parameter types, i.e basically carriers of the information in the range syntax. From that perspective the memory layout doesn't matter so much; implementations will consume the parameter and store it in their own format.

@Stebalien
Copy link
Contributor

I started working on a version that uses generics but ended up running into some limitations in the type system. Specifically, once I started introducing new types I realized that we'd need some way to handle these new types generically. Unfortunately, in order for a datastructure to support generic range indices AND usize indices, we'd either need some form of type level enum (e.g. trait enum SomeThing { Trait1 {}, Trait2 {} } where a type can be either Trait1 or Trait2 but not both), some form of type set (some of the inheritance proposals would have covered this), or negative bounds in where clauses. See below (bikeshedding on names is welcome, I'll write a formal RFC if you feel this is the right direction):

#![feature(core)]
use std::cmp::Ordering;
use std::cmp::Ord;
use std::num::Int;
use std::ops::Index;
use Bound::*;

pub struct Stuff;
// Supporting both generic range and usize indices.
impl<R: Range<usize> + !usize> Index<R> for Stuff { ... }
impl Index<usize> for Stuff { ... }

pub enum Bound<Idx> {
    Inclusive(Idx),
    Exclusive(Idx),
    Unbounded,
}
pub trait Range<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>);
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>);
}

pub struct ExclusiveRange<Idx> {
    pub start: Idx,
    pub end: Idx,
}

pub struct LeftExclusiveRange<Idx> {
    pub start: Idx,
    pub end: Idx,
}

pub struct RightExclusiveRange<Idx> {
    pub start: Idx,
    pub end: Idx,
}

pub struct InclusiveRange<Idx> {
    pub start: Idx,
    pub end: Idx,
}

pub struct ExclusiveRangeTo<Idx> {
    pub end: Idx,
}

pub struct InclusiveRangeTo<Idx> {
    pub end: Idx,
}

pub struct ExclusiveRangeFrom<Idx> {
    pub start: Idx,
}

pub struct InclusiveRangeFrom<Idx> {
    pub start: Idx,
}
pub struct RangeFull;

impl<Idx> Range<Idx> for ExclusiveRange<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Exclusive(self.start), Bound::Exclusive(self.end))
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Exclusive(&self.start), Bound::Exclusive(&self.end))
    }
}

impl<Idx> Range<Idx> for LeftExclusiveRange<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Exclusive(self.start), Bound::Inclusive(self.end))
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Exclusive(&self.start), Bound::Inclusive(&self.end))
    }
}

impl<Idx> Range<Idx> for RightExclusiveRange<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Inclusive(self.start), Bound::Exclusive(self.end))
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Inclusive(&self.start), Bound::Exclusive(&self.end))
    }
}

impl<Idx> Range<Idx> for InclusiveRange<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Inclusive(self.start), Bound::Inclusive(self.end))
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Inclusive(&self.start), Bound::Inclusive(&self.end))
    }
}

impl<Idx> Range<Idx> for ExclusiveRangeFrom<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Exclusive(self.start), Bound::Unbounded)
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Exclusive(&self.start), Bound::Unbounded)
    }
}

impl<Idx> Range<Idx> for InclusiveRangeFrom<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Inclusive(self.start), Bound::Unbounded)
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Inclusive(&self.start), Bound::Unbounded)
    }
}

impl<Idx> Range<Idx> for ExclusiveRangeTo<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Unbounded, Bound::Exclusive(self.end))
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Unbounded, Bound::Exclusive(&self.end))
    }
}

impl<Idx> Range<Idx> for InclusiveRangeTo<Idx> {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Unbounded, Bound::Inclusive(self.end))
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Unbounded, Bound::Inclusive(&self.end))
    }
}

impl<Idx> Range<Idx> for RangeFull {
    fn into_bounds(self) -> (Bound<Idx>, Bound<Idx>) {
        (Bound::Unbounded, Bound::Unbounded)
    }
    fn bounds(&self) -> (Bound<&Idx>, Bound<&Idx>) {
        (Bound::Unbounded, Bound::Unbounded)
    }
}

IMO, the only thing that really needs to be finalized before 1.0 is the name of the range structure.

@alexcrichton
Copy link
Member Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned brson Mar 23, 2015
@alexcrichton
Copy link
Member Author

Ok I talked some more with @aturon today and we believe that this is probably the route we'd take, even if we did support inclusive ranges everywhere. This means that we don't have to commit to inclusive ranges today but the door is still open to work with them tomorrow as well. Thanks for raising concerns though @Stebalien! Does that sound ok to you?

@Stebalien
Copy link
Contributor

By "this" do you mean this PR or multiple types? I'm still a little concerned about naming. The following naming scheme (or something like it) would work but is a little inconsistent (especially RangeFrom/RangeTo).

trait Bounds ... // Replaces the Range trait in my proposal. Probably a better choice anyways.

struct Range ... // [m, n)
struct RightRange ... // (m, n]
struct InclusiveRange ... // [m, n]
struct ExclusiveRange ... // (m, n)

struct RangeFrom ... // [m,infinity)
struct ExclusiveRangeFrom ... // (m, infinity)

struct RangeTo ... // (-infinity, n) //
struct InclusiveRangeTo ... // (-infinity, n]

struct FullRange ... // (-infinity, infinity)

@alexcrichton
Copy link
Member Author

Ah yes I mean this PR. I'd prefer to not delay this indefinitely until we decide on a story for inclusive ranges primarily because we don't actually know if we want all forms of inclusive ranges. I think that we can always develop a convention after-the-fact and even go the deprecation route if we really need to, but given today's context I think it's unlikely for the current names to change.

@Stebalien
Copy link
Contributor

Deprecation could be tricky (it might be doable with type aliases) but, regardless, I can live with slightly inconsistent type names. 👍

@alexcrichton
Copy link
Member Author

Ok, thanks for the input @Stebalien!

@bors: r=aturon

@bors
Copy link
Contributor

bors commented Mar 25, 2015

📌 Commit 5749a1f has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 25, 2015

⌛ Testing commit 5749a1f with merge a3b1361...

bors added a commit that referenced this pull request Mar 25, 2015
Now that we check the stability of fields, the fields of this struct should also
be stable.
@bors bors merged commit 5749a1f into rust-lang:master Mar 25, 2015
@alexcrichton alexcrichton deleted the misc-stab branch March 27, 2015 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants