Skip to content

Conversation

camsteffen
Copy link
Contributor

Closes #39511
Closes #86329

@rust-highfive
Copy link
Contributor

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2021
@scottmcm scottmcm added needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 21, 2021
@@ -601,6 +608,17 @@ where
}
}

impl<const N: usize> MultiCharEq for &[char; N] {
Copy link
Member

Choose a reason for hiding this comment

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

This might be a silly question, but why for &[char; N] instead of for [char; N]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that's what I started with, but then I figured this is more flexible/performant for large arrays. Definitely open to discussion on that. Maybe add both?

Copy link
Member

Choose a reason for hiding this comment

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

I think you do want the reference to solve those bugs where one intends to write a literal &[char] slice pattern, but it's really &[char; N].

Copy link
Member

Choose a reason for hiding this comment

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

I think both -- ['o', 'l'] for the simple case, and &[...] for the case cuviper mentions.

(I might just delegate both to the impl for slices, though, if there's an easy way to do that.)

Copy link
Contributor Author

@camsteffen camsteffen Jun 22, 2021

Choose a reason for hiding this comment

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

Okay, added both. Unfortunately lifetimes force a lot of separate structs and impls to satisfy into_searcher.

@crlf0710 crlf0710 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 9, 2021
@dtolnay
Copy link
Member

dtolnay commented Jul 26, 2021

@rust-lang/libs-api:
@rfcbot fcp merge

This PR adds impl<'a, const N: usize> Pattern<'a> for [char; N] and impl<'a, 'b, const N: usize> Pattern<'a> for &'b [char; N], which allows str::find and similar methods based on Pattern to work as:

"...".find([' ', '\r', '\n'])
// or:
"...".find(&[' ', '\r', '\n'])

whereas before, due to #39511, it would have had to be written as:

"...".find(&[' ', '\r', '\n'][..])

@rfcbot
Copy link

rfcbot commented Jul 26, 2021

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 26, 2021
@rfcbot
Copy link

rfcbot commented Jul 28, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 28, 2021
@camsteffen camsteffen force-pushed the char-array-pattern branch from c26aed0 to 28f7890 Compare July 28, 2021 21:14
@camsteffen
Copy link
Contributor Author

Squashed a fixup commit

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 7, 2021
@rfcbot
Copy link

rfcbot commented Aug 7, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 26, 2021
@camelid
Copy link
Member

camelid commented Oct 8, 2021

It looks like this PR just needs a final review now that FCP has finished.

@camelid
Copy link
Member

camelid commented Oct 8, 2021

Should relnotes be added since IIUC this is insta-stable (even though Pattern itself is not stable)?

@camsteffen
Copy link
Contributor Author

@dtolnay you approved the changes in Github. Is that grounds to r=yoself?

@camelid camelid added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 8, 2021
@camelid
Copy link
Member

camelid commented Oct 8, 2021

(Adding relnotes since this is insta-stable.)

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 30, 2021

📌 Commit 28f7890 has been approved by joshtriplett

@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 Oct 30, 2021
@bors
Copy link
Collaborator

bors commented Oct 31, 2021

⌛ Testing commit 28f7890 with merge c7e4740...

@bors
Copy link
Collaborator

bors commented Oct 31, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing c7e4740 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 31, 2021
@bors bors merged commit c7e4740 into rust-lang:master Oct 31, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 31, 2021
@jplatte jplatte mentioned this pull request Oct 31, 2021
65 tasks
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c7e4740): comparison url.

Summary: This change led to small relevant regressions 😿 in compiler performance.

  • Small regression in instruction counts (up to 0.6% on full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Oct 31, 2021
@pnkfelix
Copy link
Contributor

pnkfelix commented Nov 2, 2021

visiting for triage.

The only benchmarks that regressed did so by a small amount, percentage wise 0.2-0.6%; the benchmarks that did regress in that fashion are deeply-nested-async, helloworld, unify-linearly.

I am surprised that this change caused even this minor regression, but I don't think its worth investing effort trying to figure out why, unless someone wants to take it on as a self-educating exercise.

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. relnotes Marks issues that should be documented in the release notes of the next release. 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.

&[char;_]: doesn't implement Pattern while &[char] does Literal &[char] patterns clumsily require an as conversion