Skip to content

constify comparison traits on sliced types - #147790

Open
npmccallum wants to merge 1 commit into
rust-lang:mainfrom
npmccallum:slices
Open

constify comparison traits on sliced types#147790
npmccallum wants to merge 1 commit into
rust-lang:mainfrom
npmccallum:slices

Conversation

@npmccallum

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

Copy link
Copy Markdown
Contributor

View all comments

Constify comparison traits for str, CStr, ByteStr, and Location.

The slice comparison traits originally included here have since landed independently, so this update drops that already-merged commit and rebases the remaining constifications.

r? clarfonthey

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 Outdated
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

This comment has been minimized.

Sign in to view
@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

This comment has been minimized.

Sign in to view
@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 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.

@Randl

Randl commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

I have fixed the array comparison directly #156216

A less brittle fix may be possible but I didn't have time to think of one
I think slice comparison is fairly important to consider merging

@rustbot

rustbot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main 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.

@npmccallum npmccallum changed the title constify comparison traits on slices constify comparison traits on sliced types Sep 4, 2026
@npmccallum

npmccallum commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and narrowed the PR to the remaining str, CStr, ByteStr, and Location constifications; the slice trait changes have landed independently. I also replaced the old Location::cmp match workaround with const closures.

Validated locally with:

  • ./x test tidy
  • ./x test library/core

@rustbot

rustbot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

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