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

Split core/str/mod.rs to smaller files #76325

Merged
merged 7 commits into from
Oct 1, 2020
Merged

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 4, 2020

Note for reviewer:

  • I split to multiple commits for easier reviewing, but I could git squash them all to one if requested.
  • Recommend pulling this change locally and using advanced git diff viewer or this command:
    git show --reverse --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space master..

I split core/str/mod.rs to these modules:

  • converts: Contains helper functions to convert from bytes to str.
  • error: For error structs like Utf8Error.
  • iter: For iterators of many str methods.
  • traits: For indexing operations and build in traits on str.
  • validations: For functions validating utf8 --- This name is awkward, maybe utf8.rs is better.

@tesuji tesuji marked this pull request as ready for review September 11, 2020 11:03
@tesuji
Copy link
Contributor Author

tesuji commented Sep 11, 2020

@rustbot modify labels: +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2020
@jonas-schievink
Copy link
Contributor

r? @KodrAus

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue. 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 Sep 15, 2020
@bors
Copy link
Contributor

bors commented Sep 20, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tesuji
Copy link
Contributor Author

tesuji commented Sep 20, 2020

Rebased.

@bors
Copy link
Contributor

bors commented Sep 26, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tesuji
Copy link
Contributor Author

tesuji commented Sep 26, 2020

Rebased.

@varkor
Copy link
Member

varkor commented Sep 30, 2020

@rust-lang/libs: is there someone who could review this PR? It bitrots quite quickly.

@bors p=100

@Amanieu
Copy link
Member

Amanieu commented Sep 30, 2020

@bors r+

Also I learned something new about git today!

@bors
Copy link
Contributor

bors commented Sep 30, 2020

📌 Commit dce7248 has been approved by Amanieu

@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 Sep 30, 2020
@bors
Copy link
Contributor

bors commented Sep 30, 2020

⌛ Testing commit dce7248 with merge 9bb55dc...

@bors
Copy link
Contributor

bors commented Oct 1, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing 9bb55dc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2020
@bors bors merged commit 9bb55dc into rust-lang:master Oct 1, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 1, 2020
@tesuji tesuji deleted the split-core-str branch October 1, 2020 01:35
@bjorn3
Copy link
Member

bjorn3 commented Oct 1, 2020

@tesuji
Copy link
Contributor Author

tesuji commented Oct 1, 2020

It also regresses wall-time and some max-rss tests: https://perf.rust-lang.org/compare.html?start=ef663a8a48ea6b98b43cbfaefd99316b36b16825&end=9bb55dc8642d811d66a7599812009cc063577e00&stat=wall-time

I don't think we could do anything to improve it.

@SimonSapin
Copy link
Contributor

@bjorn3 Is that number really due to this PR? It looks like that benchmark has been oscillating between two results, recently:

image

@bjorn3
Copy link
Member

bjorn3 commented Oct 1, 2020

Ah, ok.

pub trait FromStr: Sized {
/// The associated error which can be returned from parsing.
#[stable(feature = "rust1", since = "1.0.0")]
type Err;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this was supposed to be type Error, is it possible to change this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are 9.5 years too late to change this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate: It was already type Err before this PR. #21718 which stabilized FromStr introduced type Err.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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
Development

Successfully merging this pull request may close these issues.