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

Implement `From<Cow<str>> for String` and `From<Cow<[T]>> for Vec<T>`. #37326

Merged
merged 1 commit into from Oct 23, 2016

Conversation

Projects
None yet
8 participants
@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 21, 2016

Motivation: the selectors crate is generic over a string type, in order to support all of String, string_cache::Atom, and gecko_string_cache::Atom. Multiple trait bounds are used for the various operations done with these strings. One of these operations is creating a string (as efficiently as possible, re-using an existing memory allocation if possible) from Cow<str>.

The std::convert::From trait seems natural for this, but the relevant implementation was missing before this PR. To work around this I’ve added a FromCowStr trait in selectors, but with trait coherence that means one of selectors or string_cache needs to depend on the other to implement this trait. Using a trait from std would solve this.

The Vec<T> implementation is just added for consistency. I also tried a more general impl<'a, O, B: ?Sized + ToOwned<Owned=O>> From<Cow<'a, B>> for O, but (the compiler thinks?) it conflicts with From<T> for T the impl (after moving all of collections::borrow into core::borrow to work around trait coherence).

Implement `From<Cow<str>> for String` and `From<Cow<[T]>> for Vec<T>`.
Motivation: the `selectors` crate is generic over a string type,
in order to support all of `String`, `string_cache::Atom`, and
`gecko_string_cache::Atom`. Multiple trait bounds are used
for the various operations done with these strings.
One of these operations is creating a string (as efficiently as possible,
re-using an existing memory allocation if possible) from `Cow<str>`.

The `std::convert::From` trait seems natural for this, but
the relevant implementation was missing before this PR.
To work around this I’ve added a `FromCowStr` trait in `selectors`,
but with trait coherence that means one of `selectors` or `string_cache`
needs to depend on the other to implement this trait.
Using a trait from `std` would solve this.

The `Vec<T>` implementation is just added for consistency.
I also tried a more general
`impl<'a, O, B: ?Sized + ToOwned<Owned=O>> From<Cow<'a, B>> for O`,
but (the compiler thinks?) it conflicts with `From<T> for T` the impl
(after moving all of `collections::borrow` into `core::borrow`
to work around trait coherence).
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 21, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 21, 2016

@bors: r+

cc @rust-lang/libs, new From impls!

In general we're pretty much always on board with adding various From impls afaik.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 21, 2016

📌 Commit 7e603d4 has been approved by alexcrichton

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Oct 21, 2016

@alexcrichton Wait, last time we regretted it, see #36685.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 21, 2016

@bors: r-

Indeed! Let's get a crater run this time.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Oct 22, 2016

Crater report looks clean.

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 22, 2016

📌 Commit 7e603d4 has been approved by alexcrichton

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 22, 2016

Rollup merge of rust-lang#37326 - SimonSapin:from-cow, r=alexcrichton
Implement `From<Cow<str>> for String` and `From<Cow<[T]>> for Vec<T>`.

Motivation: the `selectors` crate is generic over a string type, in order to support all of `String`,  `string_cache::Atom`, and `gecko_string_cache::Atom`. Multiple trait bounds are used for the various operations done with these strings. One of these operations is creating a string (as efficiently as possible, re-using an existing memory allocation if possible) from `Cow<str>`.

The `std::convert::From` trait seems natural for this, but the relevant implementation was missing before this PR. To work around this I’ve added a `FromCowStr` trait in `selectors`, but with trait coherence that means one of `selectors` or `string_cache` needs to depend on the other to implement this trait. Using a trait from `std` would solve this.

The `Vec<T>` implementation is just added for consistency. I also tried a more general `impl<'a, O, B: ?Sized + ToOwned<Owned=O>> From<Cow<'a, B>> for O`, but (the compiler thinks?) it conflicts with `From<T> for T` the impl (after moving all of `collections::borrow` into `core::borrow` to work around trait coherence).

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 22, 2016

Rollup merge of rust-lang#37326 - SimonSapin:from-cow, r=alexcrichton
Implement `From<Cow<str>> for String` and `From<Cow<[T]>> for Vec<T>`.

Motivation: the `selectors` crate is generic over a string type, in order to support all of `String`,  `string_cache::Atom`, and `gecko_string_cache::Atom`. Multiple trait bounds are used for the various operations done with these strings. One of these operations is creating a string (as efficiently as possible, re-using an existing memory allocation if possible) from `Cow<str>`.

The `std::convert::From` trait seems natural for this, but the relevant implementation was missing before this PR. To work around this I’ve added a `FromCowStr` trait in `selectors`, but with trait coherence that means one of `selectors` or `string_cache` needs to depend on the other to implement this trait. Using a trait from `std` would solve this.

The `Vec<T>` implementation is just added for consistency. I also tried a more general `impl<'a, O, B: ?Sized + ToOwned<Owned=O>> From<Cow<'a, B>> for O`, but (the compiler thinks?) it conflicts with `From<T> for T` the impl (after moving all of `collections::borrow` into `core::borrow` to work around trait coherence).

bors added a commit that referenced this pull request Oct 22, 2016

Auto merge of #37341 - Manishearth:rollup, r=Manishearth
Rollup of 8 pull requests

- Successful merges: #37294, #37298, #37301, #37310, #37318, #37321, #37326, #37327
- Failed merges:
@cristicbz

This comment has been minimized.

Copy link
Contributor

cristicbz commented Oct 22, 2016

Are we generally OK with allocating From conversions? I was wondering if something like impl<'a, B: ?Sized + ToOwned> From<&'a B> for B::Owned { /* ... */ } would be theoretically acceptable (ignoring coherence issues etc. for now) in the context of the Entry API extension RFC.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 22, 2016

From<&'a str> for String exists and definitely allocates, for what it’s worth.

@cristicbz

This comment has been minimized.

Copy link
Contributor

cristicbz commented Oct 22, 2016

@SimonSapin Good point, thanks!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 23, 2016

⌛️ Testing commit 7e603d4 with merge febfe76...

bors added a commit that referenced this pull request Oct 23, 2016

Auto merge of #37326 - SimonSapin:from-cow, r=alexcrichton
Implement `From<Cow<str>> for String` and `From<Cow<[T]>> for Vec<T>`.

Motivation: the `selectors` crate is generic over a string type, in order to support all of `String`,  `string_cache::Atom`, and `gecko_string_cache::Atom`. Multiple trait bounds are used for the various operations done with these strings. One of these operations is creating a string (as efficiently as possible, re-using an existing memory allocation if possible) from `Cow<str>`.

The `std::convert::From` trait seems natural for this, but the relevant implementation was missing before this PR. To work around this I’ve added a `FromCowStr` trait in `selectors`, but with trait coherence that means one of `selectors` or `string_cache` needs to depend on the other to implement this trait. Using a trait from `std` would solve this.

The `Vec<T>` implementation is just added for consistency. I also tried a more general `impl<'a, O, B: ?Sized + ToOwned<Owned=O>> From<Cow<'a, B>> for O`, but (the compiler thinks?) it conflicts with `From<T> for T` the impl (after moving all of `collections::borrow` into `core::borrow` to work around trait coherence).

@bors bors merged commit 7e603d4 into rust-lang:master Oct 23, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@SimonSapin SimonSapin deleted the SimonSapin:from-cow branch Oct 26, 2016

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 26, 2016

I’ve just realized that I don’t strictly need a From<Cow<str>> bound. I can require From<String> + From<&str> and match on cows: servo/rust-selectors@a87416f

(I still think this PR is a good addition.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment