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

Stabilize std::borrow #22210

Closed
wants to merge 10 commits into from
Closed

Stabilize std::borrow #22210

wants to merge 10 commits into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Feb 12, 2015

This commit stabilizes std::borrow, making the following modifications
to catch up the API with language changes:

  • It renames BorrowFrom to Borrow, as was originally intended (but
    blocked for technical reasons), and reorders the parameters
    accordingly.
  • It moves the type parameter of ToOwned to an associated type. This
    is somewhat less flexible, in that each borrowed type must have a
    unique owned type, but leads to a significant simplification for
    Cow. Flexibility can be regained by using newtyped slices, which is
    advisable for other reasons anyway.
  • It removes the owned type parameter from Cow, making the type much
    less verbose.

The above API changes are relatively minor; the basic functionality
remains the same, and essentially the whole module is now marked
#[stable].

[breaking-change]

r? @alexcrichton @gankro

@aturon aturon force-pushed the stab-final-borrow branch 4 times, most recently from bf2b260 to a5fd359 Compare February 12, 2015 08:41
/// }
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub enum Cow<'a, B: ?Sized + 'a> where B: ToOwned, <B as ToOwned>::Owned: 'a {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why Owned now has a : 'a bound? The old implementation didn't have that. But, perhaps something changed?

@japaric
Copy link
Member

japaric commented Feb 12, 2015

Other than the Owned: 'a bound question, this patch LGTM.

@japaric
Copy link
Member

japaric commented Feb 12, 2015

(Also it's nice that IntoCow has less input parmaters now 👍. I've used it as a bound in a few places and the previous version was quite verbose: IntoCow<'static, str, String>)

@Gankra
Copy link
Contributor

Gankra commented Feb 12, 2015

Have we pinged the community on the lost flexibility from BorrowFrom? I am always surprised what they manage to do with stuff.

@aturon
Copy link
Member Author

aturon commented Feb 12, 2015

@japaric

The extra bounds were added because of #22246, which currently means we know less about associated types than we do type parameters that are used directly.

Unfortunately, the bounds I added here are overly constraining, so this change is effectively blocked on the issue above.

@gankro

Actually, thinking more clearly about it there's no real loss in flexibility, because the coherence rules already basically rule out associating a different owned type with [T] for example.

@aturon aturon mentioned this pull request Feb 13, 2015
38 tasks
@@ -0,0 +1,310 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this moved to libcollections?

@alexcrichton
Copy link
Member

This all looks good to me, thanks @aturon! I'd be fine landing ahead of fixing #22246 if it's a backwards compatible change (haven't quite figured out if it is...)

@Manishearth
Copy link
Member

Needs a rebase, sorry.

@aturon aturon mentioned this pull request Feb 18, 2015
91 tasks
@aturon aturon force-pushed the stab-final-borrow branch 2 times, most recently from 1a9467e to a99e698 Compare February 18, 2015 23:24
@alexcrichton
Copy link
Member

@bors: r+ a99e698

@aturon
Copy link
Member Author

aturon commented Feb 18, 2015

@bors: r=alexcrichton ab59ee1

This commit stabilizes `std::borrow`, making the following modifications
to catch up the API with language changes:

* It renames `BorrowFrom` to `Borrow`, as was originally intended (but
  blocked for technical reasons), and reorders the parameters
  accordingly.

* It moves the type parameter of `ToOwned` to an associated type. This
  is somewhat less flexible, in that each borrowed type must have a
  unique owned type, but leads to a significant simplification for
  `Cow`. Flexibility can be regained by using newtyped slices, which is
  advisable for other reasons anyway.

* It removes the owned type parameter from `Cow`, making the type much
  less verbose.

* Deprecates the `is_owned` and `is_borrowed` predicates in favor of
  direct matching.

The above API changes are relatively minor; the basic functionality
remains the same, and essentially the whole module is now marked
`#[stable]`.

[breaking-change]
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 19, 2015
Conflicts:
	src/libcollections/btree/map.rs
	src/libcollections/str.rs
	src/libcollections/vec.rs
	src/libcore/borrow.rs
	src/libcore/hash/mod.rs
	src/libstd/collections/hash/map.rs
	src/libstd/collections/hash/set.rs
@alexcrichton
Copy link
Member

Merged in #22541

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.

6 participants