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

str module allocates too much; proposal for improvement #5427

Closed
Kimundi opened this issue Mar 18, 2013 · 6 comments
Closed

str module allocates too much; proposal for improvement #5427

Kimundi opened this issue Mar 18, 2013 · 6 comments
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.
Milestone

Comments

@Kimundi
Copy link
Member

Kimundi commented Mar 18, 2013

I was looking over the str module and noticed that a lot of the functions there do unnecessary allocations. I counted ~20 functions that return either an ~str or ~[~str] where a &str or a bunch of &str would do. (they're mostly search / trim functions)

I'd like to change them, however I'm not sure if that would be the best way in regards to API design.

My proposal is:

  • Every function that returns ~str but could also return an &str will get changed to do so. If users want to have an explicit copy, they have to call to_uniqe() or similar.
  • Every function that returns an ~[~str] but could also return return a list of &str will get rewritten under an other name to work as an iterator that takes an fn(&str) -> bool closure. The original name still remains, and will just wrap that iterator in an ~[~str].
  • Remove the view functions, make the slice ones return &str (mirrors recent change to vec)
@catamorphism
Copy link
Contributor

Agreed. Function argument types should be as general as possible (but no more general).

@pcwalton
Copy link
Contributor

+1

@huonw
Copy link
Member

huonw commented Mar 20, 2013

I started doing something along these lines without seeing this issue. So far I've basically just done the first point (WIP, str.rs), I'm happy to continue with the rest of your proposal @Kimundi, or let you do it, either is fine. What do you think? (edit: this is resolved, Kimundi and I had a discussion on IRC: he's almost done and will continue with the work.)

@huonw
Copy link
Member

huonw commented Mar 20, 2013

#5422 seems to fit into this issue too.

@Kimundi
Copy link
Member Author

Kimundi commented Mar 21, 2013

The view <-> slice change is done now, now working on the cases that return ~str

@Kimundi
Copy link
Member Author

Kimundi commented Mar 26, 2013

All changes listed here have been implemented, closing issue.

@Kimundi Kimundi closed this as completed Mar 26, 2013
oli-obk pushed a commit to oli-obk/rust that referenced this issue May 2, 2020
Add lint named implicit_saturating_sub

Fixes: rust-lang#5399
I've made a basic skeleton of the lint, would love more feedback on how to make it better.
changelog: Add lint [`implicit_saturating_sub`]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

4 participants