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

impl FromIterator<char> for Box<str> #70094

Open
wants to merge 1 commit into
base: master
from

Conversation

@andersk
Copy link
Contributor

andersk commented Mar 18, 2020

This is analogous to impl FromIterator<A> for Box<[A]> (#55843).

Fixes #65163.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 18, 2020

r? @LukasKalbertodt

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

@andersk andersk force-pushed the andersk:boxed_str_from_iter branch 2 times, most recently from 73ea386 to 5500fcd Mar 18, 2020
Copy link
Member

LukasKalbertodt left a comment

This seems like a reasonable addition to me. Thanks!

Please add four tests for the other four impls. And see my inline comment on the test.

src/liballoc/tests.rs Outdated Show resolved Hide resolved
These are analogous to impl FromIterator<A> for Box<[A]> (#55843).

Fixes #65163.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk andersk force-pushed the andersk:boxed_str_from_iter branch from 26e3e08 to e355419 Mar 18, 2020
@andersk

This comment has been minimized.

Copy link
Contributor Author

andersk commented Mar 18, 2020

Sure, now all five impls are tested.

@andersk andersk requested a review from LukasKalbertodt Mar 18, 2020
Copy link
Member

LukasKalbertodt left a comment

Looks good to me!

@LukasKalbertodt

This comment has been minimized.

Copy link
Member

LukasKalbertodt commented Mar 18, 2020

This PR adds the following stable impls:

  • impl<'a> FromIterator<&'a char> for Box<str>
  • impl<'a> FromIterator<&'a str> for Box<str>
  • impl<'a> FromIterator<Cow<'a, str>> for Box<str>
  • impl FromIterator<String> for Box<str>
  • impl FromIterator<char> for Box<str>

All these impls already exist for String. Thus, the implementation is always simply: iter.into_iter().collect::<String>().into_boxed_str(). We also already had impl FromIterator<A> for Box<[A]> in std for a long time. There was an attempt at this already in #65168, which was closed due to inactivity.


As I cannot start FCP merges, reassigning.

@LukasKalbertodt

This comment has been minimized.

Copy link
Member

LukasKalbertodt commented Mar 18, 2020

@andersk

This comment has been minimized.

Copy link
Contributor Author

andersk commented Mar 18, 2020

Note: I saw in #40028 that @withoutboats regretted that the five String impls could not be generalized to a single impl<T> FromIterator<T> for String where String: Extend<T> due to backwards compatibility (it might overlap with impl FromIterator<Foo> for String for a custom type Foo). I believe the same concern prevents a similar simplification for Box<str>, but I figured I’d mention it in case my understanding is wrong.

@jonas-schievink jonas-schievink added this to the 1.44 milestone Mar 18, 2020
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Mar 18, 2020

As I cannot start FCP merges, reassigning.

You should send in a PR to https://github.com/rust-lang/team to fix that btw. :)

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Mar 18, 2020

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 18, 2020

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Mar 18, 2020

I am not enthusiastic about this addition. I would prefer not to go any more in the direction of making Box<str> a fully fledged string type. I see it as a niche alternative representation for owned strings for use cases in which 2 words of size instead of 3 words matters for memory usage, such as a big intern table. In general I'd like users to reach for String when operating with owned strings, and use into_boxed_str if their use case demands the representation of a boxed str.

I'm fine with the necessary APIs for Box<str> as a niche string representation, including things like From<String> and Default. I'm less fine with growing more string-related surface area around Box<str> than that, like FromIterator, Extend, Add, etc. If someone wants to collect into an owned string and use Box<str> as the representation, I think it's fine to have them write collect::<String>().into_boxed_str() instead of collect::<Box<str>>().

@rfcbot concern is Box<str> a string or a niche string representation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.