Skip to content

Conversation

cammeresi
Copy link
Contributor

@cammeresi cammeresi commented Sep 16, 2025

  • plumb in allocator generic
  • additional testing

Related: #122742

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

rustbot commented Sep 16, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@joboet
Copy link
Member

joboet commented Sep 18, 2025

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 18, 2025
@rustbot rustbot assigned BurntSushi and unassigned tgross35 Sep 18, 2025
@joboet joboet changed the title Minor improvements to Vec::peek_mut Make PeekMut generic over the allocator Sep 18, 2025
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

Looks great! This needs approval from someone in T-libs-api though, so it can't be merged just yet.

View changes since this review

@tgross35
Copy link
Contributor

tgross35 commented Sep 18, 2025

@rust-lang/libs-api anybody willing to give a quick thumbs up here? Straightforward extension to unstable API.

@cammeresi please squash when you get the chance

#[unstable(feature = "vec_peek_mut", issue = "122742")]
pub struct PeekMut<'a, T> {
vec: &'a mut Vec<T>,
pub struct PeekMut<'a, T, A: Allocator> {
Copy link
Member

Choose a reason for hiding this comment

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

The allocator parameter should be unstable and default to Global, just like that of Vec,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was going to ask. I wasn't sure since there's no public entry point by which to create one directly. I guess the point is to be defensive against such an accidental change?

Does that mean that std::collections::btree_map::IntoKeys is incorrectly missing the attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's sort of indirectly gated because there is nothing stable implementing Allocator that you could specify in that position. But yeah, I think IntoKeys should technically have the same unstable attribute. Feel free to put up a PR if you're interested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amanieu
Copy link
Member

Amanieu commented Sep 19, 2025

r=me once the generic parameter is fixed.

@cammeresi
Copy link
Contributor Author

@tgross35 Am I supposed to ping now that I've updated? (Also, if you can answer my review question, I'd appreciate it.)

@Amanieu
Copy link
Member

Amanieu commented Sep 20, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 20, 2025

📌 Commit 934ee04 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 Sep 20, 2025
@bors
Copy link
Collaborator

bors commented Sep 20, 2025

⌛ Testing commit 934ee04 with merge e4b5219...

@bors
Copy link
Collaborator

bors commented Sep 20, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2025
@bors bors merged commit e4b5219 into rust-lang:master Sep 20, 2025
11 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 20, 2025
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing ec38671 (parent) -> e4b5219 (this PR)

Test differences

Show 200 test diffs

200 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard e4b521903b3b1a671e26a70b9475bcff385767e5 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-apple-various: 5142.9s -> 3455.3s (-32.8%)
  2. dist-aarch64-windows-gnullvm: 5398.6s -> 4631.9s (-14.2%)
  3. dist-aarch64-apple: 6123.4s -> 6750.9s (10.2%)
  4. dist-x86_64-windows-gnullvm: 4688.0s -> 5054.6s (7.8%)
  5. dist-x86_64-apple: 7693.0s -> 7129.6s (-7.3%)
  6. x86_64-gnu-aux: 6373.1s -> 6800.4s (6.7%)
  7. dist-arm-linux-musl: 5440.5s -> 5803.0s (6.7%)
  8. dist-powerpc64le-linux-gnu: 5567.0s -> 5222.7s (-6.2%)
  9. dist-powerpc64-linux: 5029.0s -> 5323.5s (5.9%)
  10. x86_64-msvc-ext3: 6339.9s -> 6699.7s (5.7%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e4b5219): 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)

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

Cycles

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

Binary size

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

Bootstrap: 470.599s -> 472.018s (0.30%)
Artifact size: 389.97 MiB -> 389.98 MiB (0.00%)

@cammeresi cammeresi deleted the peek-20250915 branch September 20, 2025 15:28
Muscraft pushed a commit to Muscraft/rust that referenced this pull request Sep 24, 2025
Make `PeekMut` generic over the allocator

- plumb in allocator generic
- additional testing

Related: rust-lang#122742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants