-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Impl String::into_chars #133057
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 String::into_chars #133057
Conversation
Failed to set assignee to
|
library/alloc/src/string.rs
Outdated
None => None, | ||
Some((_, ch)) => { | ||
let offset = iter.offset(); | ||
drop(self.bytes.drain(..offset)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me of this thread that suggests a truncate_front
method on Vec
. Not sure if it's desired as a separate proposal.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks for your review! It seems this direction is good to go. I'd create a tracking issue, update the attr in code and add docs with doctests, then request a review for merging. Hopefully in this week. |
This PR is ready for merge from my perspective. Ping @programmerjake @kennytm @Amanieu if you have time to give it a review :D I'd still wonder whether we'd add a String::into_char_indices method also? Seems intuitive and for symmetry. |
This comment has been minimized.
This comment has been minimized.
Looks good, aside from some small nits:
|
@thaliaarchi Thank you! Comments addressed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
iirc squashing is not a requirement, so if you feel you're losing important commit history, just don't squash |
@programmerjake Thanks for your information! I was told to do this sometime and not sure if there is a consensus or reference. |
you don't need to feel like I'm telling you what you must do or anything, I'm not one of the people who can approve PRs for this repo anyway so my reviews are merely advisory (I only have that privilege for the portable-simd repos). That said, PRs can normally take more than a month and also this is holiday season (so people may be on vacation and those that aren't may have reduced capacity) and is right before the 2024 edition comes out so taking longer than usual is expected. |
This comment has been minimized.
This comment has been minimized.
I've seen a number of PRs that were merged with multiple commits (e.g. just looking through |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: tison <[email protected]>
Hello @Amanieu! May we have another review around? |
Sorry for the delay, I'm currently going through my PR review backlog. @bors r+ |
Tracking issue - #133125
r? @programmerjake @kennytm @Amanieu
This refers to rust-lang/libs-team#268
Before adding tests and creating a tracking issue, I'd like to reach a consensus on the implementation direction and two questions:
String::into_char_indices
method also?