Skip to content

constify comparison traits on slices - #147790

Open
npmccallum wants to merge 2 commits into
rust-lang:mainfrom
npmccallum:slices
Open

constify comparison traits on slices#147790
npmccallum wants to merge 2 commits into
rust-lang:mainfrom
npmccallum:slices

Conversation

@npmccallum

@npmccallum npmccallum commented Oct 16, 2025 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

r? oli-obk

Tracking issue #143800

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 16, 2025
@rustbot

rustbot commented Oct 16, 2025

Copy link
Copy Markdown
Collaborator

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

oli-obk
oli-obk requested changes Oct 19, 2025
View reviewed changes

@oli-obk oli-obk left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some nits

View changes since this review

Comment thread library/core/src/panic/location.rs
Comment thread library/core/src/slice/cmp.rs
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2025
@rustbot

This comment has been minimized.

Sign in to view
oli-obk
oli-obk reviewed Oct 23, 2025
View reviewed changes
Comment thread library/core/src/panic/location.rs Outdated
@rustbot

rustbot commented Oct 24, 2025

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@oli-obk

oli-obk commented Oct 24, 2025

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Oct 24, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit 4b1d118 has been approved by oli-obk

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Oct 25, 2025
@jhpratt
Rollup merge of rust-lang#147790 - npmccallum:slices, r=oli-obk
7613af6 
constify comparison traits on slices

r? oli-obk

Tracking issue rust-lang#143800
@jhpratt jhpratt mentioned this pull request Oct 25, 2025
Closed
bors added a commit that referenced this pull request Oct 25, 2025
@bors
Auto merge of #148087 - jhpratt:rollup-7kiyutg, r=jhpratt
5b977f5 
Rollup of 6 pull requests

Successful merges:

 - #147406 (Remove needs-asm-support directive in tests with explicit targets)
 - #147790 (constify comparison traits on slices)
 - #148056 (refactor(rustdoc): Remove redundant langstr checks)
 - #148065 (compiletest: Add concrete examples for some config/test path fields)
 - #148072 (Fix compiling `CondVar::wait_timeout` on 32-bit Apple platforms)
 - #148073 (test(frontmatter): Rename tests to make coverage more obvious)

r? `@ghost`
`@rustbot` modify labels: rollup
@jhpratt

jhpratt commented Oct 25, 2025

Copy link
Copy Markdown
Member

@bors r- #148087 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 25, 2025
@Randl

Randl commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

Changing slices PartialEq implementation breaks codegen tests, I've encountered this in the past
#144847 (comment)

@rust-bors

rust-bors Bot commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

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

@SpriteOvO

SpriteOvO commented May 3, 2026

Copy link
Copy Markdown
Member

@npmccallum Hi, ping from triage team. This PR has been inactive for a while. Are there any updates on this? Thanks.

@npmccallum

npmccallum commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Picking this back up after the triage ping — sorry for the delay. Before I put work into it, I'd like to check whether it's still wanted.

State of the PR: it was r+'d, then r-'d because tests/codegen-llvm/array-cmp.rs fails on aarch64-gnu; @Randl notes that changing the slice PartialEq impl breaks that test reliably (#144847 (comment)). It has since gone out of date. That's a tractable fix — rebase and update the codegen expectations.

But the r+ predates #155816, which libs has now signed off on. This PR is completeness-driven rather than driven by a need inside rust-lang, so I think it plausibly falls on the "decline for now" side of those guidelines. I've just closed #146097 on that basis.

@oli-obk @tgross35 — does the approval still stand under the new policy? If yes I'll rebase and sort out the codegen test; if not I'll close this too, no hard feelings.

@clarfonthey

clarfonthey commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

From the policy's perspective, the main issue with this PR is that it introduces new const hacks, although I believe that since const closures are now a thing those hacks should no longer be required. Since comparisons between slices are useful in a const context and most of this is just adding a stability marker, it would be fine to merge.

r? clarfonthey

Would be fine accepting this if you want to rebase & remove the hacks, also fine if you want to open a new PR instead. I would recommend just doing r? libs for new constifications since if it gets to the level of complexity where wg-const-traits needs to intervene it probably is too complicated to accept.

@rustbot rustbot assigned clarfonthey and Mark-Simulacrum and unassigned oli-obk Aug 8, 2026
@clarfonthey

clarfonthey commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

I… 🤦🏻

Technically did ask for a second reviewer from libs, didn't I. Unassigning Mark since that was a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obk oli-obk oli-obk requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@clarfonthey clarfonthey

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@npmccallum @rustbot @oli-obk @bors @jhpratt @Randl @SpriteOvO @clarfonthey @Mark-Simulacrum