Skip to content

Split*::as_str refactor #95644

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 3, 2023
Merged

Split*::as_str refactor #95644

merged 2 commits into from
Jan 3, 2023

Conversation

@WaffleLapkin
Copy link
Member

I've made this patch almost a year ago, so the rename and the behavior change are in one commit, sorry 😅

This fixes #84974, as it's required to make other changes work.

This PR

  • Renames as_str method of string Split* iterators to remainder (it seems like the as_str name was confusing to users)
  • Makes remainder return Option<&str>, to distinguish between "the iterator is exhausted" and "the tail is empty", this was required on the tracking issue

r? @m-ou-se

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2022
klensy
klensy reviewed Apr 4, 2022
View reviewed changes
@WaffleLapkin WaffleLapkin force-pushed the str_split_as_str_refactor_take2 branch from b52f35b to 13bc86d Compare April 4, 2022 13:34
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 23, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@jplatte jplatte mentioned this pull request Jun 24, 2022
Closed
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
@JohnCSimon
Copy link
Member

@klensy @m-ou-se this is still waiting on review

@bors
Copy link
Collaborator

bors commented Nov 21, 2022

☔ The latest upstream changes (presumably #104655) made this pull request unmergeable. Please resolve the merge conflicts.

@WaffleLapkin
SplitInternal: always set finished in get_end
ca4989e
@WaffleLapkin
Replace Split*::as_str with remainder
b458a49 
This commit
- Renames `Split*::{as_str -> remainder}` as it seems less confusing
- Makes `remainder` return Option<&str> to distinguish between
  "iterator is exhausted" and "the tail is empty"
@rust-cloud-vms rust-cloud-vms bot force-pushed the str_split_as_str_refactor_take2 branch from 13bc86d to b458a49 Compare December 16, 2022 13:06
@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@WaffleLapkin WaffleLapkin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 16, 2022
@WaffleLapkin
Copy link
Member Author

r? libs-api

@rustbot rustbot assigned Amanieu and unassigned m-ou-se Dec 16, 2022
@Amanieu
Copy link
Member

Amanieu commented Jan 3, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 3, 2023

📌 Commit b458a49 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2023
@bors
Copy link
Collaborator

bors commented Jan 3, 2023

⌛ Testing commit b458a49 with merge b435960...

@bors
Copy link
Collaborator

bors commented Jan 3, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing b435960 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 3, 2023
@bors bors merged commit b435960 into rust-lang:master Jan 3, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 3, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b435960): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 3.0% [3.0%, 3.0%] 1
Regressions ❌
(secondary)		 1.1% [1.1%, 1.1%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.0% [3.0%, 3.0%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@WaffleLapkin WaffleLapkin deleted the str_split_as_str_refactor_take2 branch January 3, 2023 16:14
@WaffleLapkin WaffleLapkin mentioned this pull request Jan 3, 2023
Open
3 tasks
overvenus added a commit to overvenus/tikv that referenced this pull request Dec 22, 2023
@overvenus
*: upgrade str_split_remainder
0efbda5 
See rust-lang/rust/pull/95644

Signed-off-by: Neil Shen <[email protected]>
@dtolnay dtolnay mentioned this pull request Nov 24, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@klensy klensy klensy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Amanieu Amanieu

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.68.0

Development

Successfully merging this pull request may close these issues.

str::split_terminator is never finished

10 participants

@WaffleLapkin @JohnCSimon @bors @rustbot @Amanieu @rust-timer @klensy @m-ou-se @apiraino @rust-highfive