Remove &[T] from vec_deque::Drain #101299

saethlin · 2022-09-02T00:32:03Z

I don't know what the right approach is here. There were a few suggestions in the issue, and they all seem a bit thorny to implement. So I just picked one that was kind of familiar.

rust-highfive · 2022-09-02T00:32:07Z

r? @joshtriplett

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

library/alloc/src/collections/vec_deque/iter.rs

SkiFire13 · 2022-09-02T14:10:40Z

library/alloc/src/collections/vec_deque/iter.rs

 }

 impl<'a, T> Iter<'a, T> {
    pub(super) fn new(ring: &'a [MaybeUninit<T>], tail: usize, head: usize) -> Self {
-        Iter { ring, tail, head }
+        Iter {
+            ring: ptr::slice_from_raw_parts(ring.as_ptr().cast(), ring.len()),


This could also be written as ring as *const [MaybeUninit<T>] as *const [T]. Not sure if the as cast is better than a ptr::slice_from_raw_parts call, but in both cases I feel like there should be a better way to write this.

library/alloc/src/collections/vec_deque/iter.rs

RalfJung · 2022-09-09T10:54:51Z

r? libs

RalfJung · 2022-09-09T10:55:57Z

library/alloc/src/collections/vec_deque/iter.rs

+        }
+    }
+
+    unsafe fn ring(&self) -> &'a [MaybeUninit<T>] {


That's a very long-lived reference, is that needed?

In particular, the entire reason this PR is made is that this lifetime is a lie -- when Iter is used in Drain and Drain is dropped, the memory will be deallocated while 'a is still ongoing. That should probably be documented somewhere.

As far as I can tell, we need to get a &'a T by some means in order to implement all the iterator methods, so we're stuck with this hazard.

Let's see if I can do something to just Drain to fix the problem there...

The second commit in this PR now reverts the first one and modifies just the Drain struct. Do you think that approach is more satisfying?

I haven't checked the details but I do think that reusing Iter was kind of a hack so this seems cleaner to me indeed.

Let's see what the libs reviewer says.

Seems fine to me.

rustbot · 2022-09-10T21:54:56Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

thomcc · 2022-09-11T19:50:33Z

Thanks.

@bors r+

bors · 2022-09-11T19:50:35Z

📌 Commit 54684c4 has been approved by thomcc

It is now in the queue for this repository.

bors · 2022-09-11T19:50:43Z

⌛ Testing commit 54684c4 with merge 59e7a30...

bors · 2022-09-11T22:19:17Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 59e7a30 to master...

rust-timer · 2022-09-11T23:36:18Z

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

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rust-highfive assigned joshtriplett Sep 2, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2022

saethlin commented Sep 2, 2022

View reviewed changes

library/alloc/src/collections/vec_deque/iter.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

saethlin force-pushed the vecdeque-drain-drop branch from 05fffed to 2ef7b98 Compare September 2, 2022 00:37

SkiFire13 reviewed Sep 2, 2022

View reviewed changes

5225225 reviewed Sep 2, 2022

View reviewed changes

library/alloc/src/collections/vec_deque/iter.rs Outdated Show resolved Hide resolved

saethlin force-pushed the vecdeque-drain-drop branch from 2ef7b98 to f698c27 Compare September 2, 2022 23:16

This comment has been minimized.

Sign in to view

saethlin force-pushed the vecdeque-drain-drop branch from 848f7cd to 366b647 Compare September 4, 2022 03:23

rust-highfive assigned thomcc and unassigned joshtriplett Sep 9, 2022

RalfJung reviewed Sep 9, 2022

View reviewed changes

saethlin added 2 commits September 10, 2022 17:52

Remove &[T] from vec_deque::Drain

25f4cb5

Alternate approach; just modify Drain

54684c4

saethlin force-pushed the vecdeque-drain-drop branch from 366b647 to 54684c4 Compare September 10, 2022 21:54

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 11, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2022

bors merged commit 59e7a30 into rust-lang:master Sep 11, 2022

rustbot added this to the 1.65.0 milestone Sep 11, 2022

saethlin deleted the vecdeque-drain-drop branch September 21, 2022 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove &[T] from vec_deque::Drain #101299

Remove &[T] from vec_deque::Drain #101299

saethlin commented Sep 2, 2022

rust-highfive commented Sep 2, 2022

This comment has been minimized.

SkiFire13 Sep 2, 2022

This comment has been minimized.

RalfJung commented Sep 9, 2022

RalfJung Sep 9, 2022 •

edited

Loading

saethlin Sep 10, 2022

saethlin Sep 10, 2022

RalfJung Sep 11, 2022

thomcc Sep 11, 2022

rustbot commented Sep 10, 2022

thomcc commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

rust-timer commented Sep 11, 2022

Remove &[T] from vec_deque::Drain #101299

Remove &[T] from vec_deque::Drain #101299

Conversation

saethlin commented Sep 2, 2022

rust-highfive commented Sep 2, 2022

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung commented Sep 9, 2022

RalfJung Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Sep 10, 2022

thomcc commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

rust-timer commented Sep 11, 2022

Overall result: no relevant changes - no action needed

Footnotes

RalfJung Sep 9, 2022 •

edited

Loading