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

RFC: Change return type of str::replace to MaybeOwned to avoid a copy #33

Closed

Conversation

SimonSapin
Copy link
Contributor

No description provided.

@alexcrichton
Copy link
Member

I am slightly worried about the usage of this MaybeOwned type. It seems like a bit of a hack to get around allocating in a few situations.

This may not be the last method to receive the MaybeOwned treatment. Taking a look at the available methods, it looks like escape_default would be another possible candidate for this (there may be more as well). If it starts getting used in more functions, it will be expected to be used everywhere.

@lilyball
Copy link
Contributor

lilyball commented Apr 5, 2014

@alexcrichton I'm tentatively in favor of using MaybeOwned on all methods that return a ~str but don't necessarily have to allocate. My primary concern is that this will just lead to a lot more calls to .as_slice() and .into_owned().

This would be somewhat alleviated if MaybeOwned implemented all the same methods that &str does, but that's a maintenance burden. This might be a good reason to go ahead and write a variant of deriving that allows for delegating the trait impl to the results of a method call on self (so MaybeOwned could just be marked up as something like #[delegating(Str,as_slice)] and rustc would keep it up to date).

@huonw
Copy link
Member

huonw commented Apr 5, 2014

We can have iterators too, so there's very very rarely an allocation (e.g. replace<'a>(&'a self, replacement: &'a str) -> Iterator<&'a str>), although this is probably relatively hard to use.

@emberian
Copy link
Member

emberian commented Apr 5, 2014

MaybeOwned could implement Deref<str> once we have DST, thus essentially always acting like a slice. I'm generally in favor of using MaybeOwned where allocations aren't necessary.

Also, I don't think this deserves an RFC. RFCs should be for language changes and changes to items marked #[stable], I think.

@SimonSapin
Copy link
Contributor Author

Also, I don't think this deserves an RFC. RFCs should be for language changes and changes to items marked #[stable], I think.

Filed #35 about this.

@SimonSapin
Copy link
Contributor Author

MaybeOwned could implement Deref<str> once we have DST, thus essentially always acting like a slice.

I like this idea. Can’t wait for DST :)

@nikomatsakis
Copy link
Contributor

I do think this deserves an RFC, in the sense that it represents a broader API decision. Also, while str and vec are not technically stable, they are probably as close as anything we have (though we can expect some post-DST mixup in the traits and so forth).

@brson
Copy link
Contributor

brson commented Jun 11, 2014

Discussed at https://github.com/mozilla/rust/wiki/Meeting-weekly-2014-06-10

There's not a lot of love for MaybeOwned and nobody wants to expand it's use. We'd like to wait and see if a more holistic approach to solving the maybe-owned problem emerges.

@brson brson closed this Jun 11, 2014
@SimonSapin
Copy link
Contributor Author

@aturon Is it worth reviving this RFC, with s/MaybeOwned/CowString/ ?

@aturon
Copy link
Member

aturon commented Dec 10, 2014

This is definitely worth revisiting, now that our clone-on-write plans are clear. In particular, we need a clear convention for when Cow-related types/traits are appropriate. I'd be happy to work with you to draft such a thing.

@aturon
Copy link
Member

aturon commented Dec 10, 2014

cc @alexcrichton

@aturon
Copy link
Member

aturon commented Dec 10, 2014

An interesting note: it's generally possible to go from taking owned/borrowed strings as inputs and accepting IntoCow later (without breakage). But that's not true for return types. So the thing we most need to clarify is when it's appropriate to return a Cow.

@SimonSapin
Copy link
Contributor Author

For what it’s worth, there is a precedent of returning CowString in String::from_utf8_lossy, whose algorithm is kinda similar to that of replace.

@alexcrichton
Copy link
Member

I agree with @aturon that we should probably have a strong set of conventions for returning Cow from methods before pushing it into replace.

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.

8 participants