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

FromIterator and Extend Cow for String #41449

Merged
merged 1 commit into from Apr 30, 2017

Conversation

Projects
None yet
9 participants
@Eh2406
Contributor

Eh2406 commented Apr 21, 2017

This is a quick draft to start working on #41351.
I don't think I got the stable attributes correct, but it is good enuf to start a conversation.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Apr 21, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

Collaborator

rust-highfive commented Apr 21, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

@mrhota

This comment has been minimized.

Show comment
Hide comment
@mrhota

mrhota Apr 21, 2017

Contributor

@Eh2406 can you respond to the comment on the linked issue? Why not impl FromIterator<Borrow<str>> for String?

Contributor

mrhota commented Apr 21, 2017

@Eh2406 can you respond to the comment on the linked issue? Why not impl FromIterator<Borrow<str>> for String?

@jmesmon

This comment has been minimized.

Show comment
Hide comment
@jmesmon

jmesmon Apr 21, 2017

Contributor

On the stability attributes: Trait impls are (unfortunately) still insta-stable, they ignore stability attributes.

Edit: oh, and it would really need to be impl<T: Borrow<str>> FromIterator<T> for String to be a reasonable substitution.

Contributor

jmesmon commented Apr 21, 2017

On the stability attributes: Trait impls are (unfortunately) still insta-stable, they ignore stability attributes.

Edit: oh, and it would really need to be impl<T: Borrow<str>> FromIterator<T> for String to be a reasonable substitution.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Apr 21, 2017

Contributor

I'd love to use the more generic version. It will replace all existing impls. Do we just throw out the stability feature and versions?

Contributor

Eh2406 commented Apr 21, 2017

I'd love to use the more generic version. It will replace all existing impls. Do we just throw out the stability feature and versions?

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Apr 21, 2017

Contributor

char can not impl Borrow thus the generic does not cover existing stable uses, but rust does not know that it will never impl it so it won't let us have the generic and the specific.

// proposed generic version
impl<T: Borrow<str>> FromIterator<T> for String {}

// existing impls
impl<'a> FromIterator<&'a str> for String {...} // remove, covered by generic 
impl FromIterator<String> for String {...} // remove, covered by generic 
impl FromIterator<char> for String {...} // not covered nor compatible with generic 
Contributor

Eh2406 commented Apr 21, 2017

char can not impl Borrow thus the generic does not cover existing stable uses, but rust does not know that it will never impl it so it won't let us have the generic and the specific.

// proposed generic version
impl<T: Borrow<str>> FromIterator<T> for String {}

// existing impls
impl<'a> FromIterator<&'a str> for String {...} // remove, covered by generic 
impl FromIterator<String> for String {...} // remove, covered by generic 
impl FromIterator<char> for String {...} // not covered nor compatible with generic 
@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Apr 22, 2017

Contributor

and if I remove the impls for char it corectly points out that the trait core::borrow::Borrow<str> is not implemented for char. So I don't see how to do this genericly.

Contributor

Eh2406 commented Apr 22, 2017

and if I remove the impls for char it corectly points out that the trait core::borrow::Borrow<str> is not implemented for char. So I don't see how to do this genericly.

@jmesmon

This comment has been minimized.

Show comment
Hide comment
@jmesmon

jmesmon Apr 24, 2017

Contributor

hmm, that is a shame about the conflicts. Can specialization be used here to allow this? (And I suppose the follow up is: do we want to use specialization for this case).

Contributor

jmesmon commented Apr 24, 2017

hmm, that is a shame about the conflicts. Can specialization be used here to allow this? (And I suppose the follow up is: do we want to use specialization for this case).

@Eh2406 Eh2406 referenced this pull request Apr 24, 2017

Closed

Gen herd cows #41506

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Apr 24, 2017

Contributor

conflicting implementations but this is my ferst use of specialization. Did I use it wrong?

I have bean testing in other prs so that this direct version says mergeable.

Contributor

Eh2406 commented Apr 24, 2017

conflicting implementations but this is my ferst use of specialization. Did I use it wrong?

I have bean testing in other prs so that this direct version says mergeable.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Apr 25, 2017

Contributor

Thanks for the PR! We'll make sure that @aturon (or someone else) helps you with the specialization and reviews this.

r? @aturon

Contributor

arielb1 commented Apr 25, 2017

Thanks for the PR! We'll make sure that @aturon (or someone else) helps you with the specialization and reviews this.

r? @aturon

@arielb1 arielb1 assigned aturon and unassigned sfackler Apr 25, 2017

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 29, 2017

Member

Specialization can't be used to tackle this problem, because char does not implement the necessary trait (and hence the impl isn't a specialization).

The core problem is as @Eh2406 identified, which is that Rust is trying to defend against a future where char does impl the trait (this happens because char is defined in core while this impl must live in std).

I think this is the best we can do for the time being. Can you update the version numbers on the stability attributes, to 1.19?

Otherwise, r=me.

Member

aturon commented Apr 29, 2017

Specialization can't be used to tackle this problem, because char does not implement the necessary trait (and hence the impl isn't a specialization).

The core problem is as @Eh2406 identified, which is that Rust is trying to defend against a future where char does impl the trait (this happens because char is defined in core while this impl must live in std).

I think this is the best we can do for the time being. Can you update the version numbers on the stability attributes, to 1.19?

Otherwise, r=me.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Apr 29, 2017

Contributor

Fixed, rebased, and squashed.

Contributor

Eh2406 commented Apr 29, 2017

Fixed, rebased, and squashed.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 29, 2017

Member

Thanks!

@bors: r+ rollup

Member

aturon commented Apr 29, 2017

Thanks!

@bors: r+ rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 29, 2017

Contributor

📌 Commit 0e2571b has been approved by aturon

Contributor

bors commented Apr 29, 2017

📌 Commit 0e2571b has been approved by aturon

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 29, 2017

Rollup merge of #41449 - Eh2406:master, r=aturon
FromIterator and Extend Cow for String

This is a quick draft to start working on [#41351](rust-lang#41351).
I don't think I got the stable attributes correct, but it is good enuf to start a conversation.

bors added a commit that referenced this pull request Apr 29, 2017

Auto merge of #41634 - frewsxcv:rollup, r=frewsxcv
Rollup of 5 pull requests

- Successful merges: #41449, #41509, #41608, #41613, #41618
- Failed merges:

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 29, 2017

Rollup merge of #41449 - Eh2406:master, r=aturon
FromIterator and Extend Cow for String

This is a quick draft to start working on [#41351](rust-lang#41351).
I don't think I got the stable attributes correct, but it is good enuf to start a conversation.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 30, 2017

Rollup merge of #41449 - Eh2406:master, r=aturon
FromIterator and Extend Cow for String

This is a quick draft to start working on [#41351](rust-lang#41351).
I don't think I got the stable attributes correct, but it is good enuf to start a conversation.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 30, 2017

Rollup merge of #41449 - Eh2406:master, r=aturon
FromIterator and Extend Cow for String

This is a quick draft to start working on [#41351](rust-lang#41351).
I don't think I got the stable attributes correct, but it is good enuf to start a conversation.

bors added a commit that referenced this pull request Apr 30, 2017

Auto merge of #41643 - frewsxcv:rollup, r=frewsxcv
Rollup of 6 pull requests

- Successful merges: #41449, #41509, #41608, #41613, #41636, #41637
- Failed merges:

@bors bors merged commit 0e2571b into rust-lang:master Apr 30, 2017

1 check passed

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

bors bot added a commit to rayon-rs/rayon that referenced this pull request Jan 14, 2018

Merge #497 #498
497: impl FromParallelIterator<()> for () r=cuviper a=cuviper

This is more useful when combined with higher-level abstractions, like
collecting to a `Result<(), E>` where you only care about errors.

This is a parallel version of rust-lang/rust#45379.
Cc #496

498: FromParallelIterator and ParallelExtend Cow for String r=cuviper a=cuviper

Parallel version of rust-lang/rust#41449.

bors bot added a commit to rayon-rs/rayon that referenced this pull request Jan 14, 2018

Merge #498
498: FromParallelIterator and ParallelExtend Cow for String r=cuviper a=cuviper

Parallel version of rust-lang/rust#41449.

bors bot added a commit to rayon-rs/rayon that referenced this pull request Jan 14, 2018

Merge #498
498: FromParallelIterator and ParallelExtend Cow for String r=cuviper a=cuviper

Parallel version of rust-lang/rust#41449.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment