Add back Send and Sync impls on ChunksMut iterators #100023

saethlin · 2022-08-01T13:27:25Z

These were accidentally removed in #94247 because the representation was changed from &mut [T] to *mut T, which has !Send + !Sync.

rustbot · 2022-08-01T13:27:28Z

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

rust-highfive · 2022-08-01T13:27:28Z

r? @m-ou-se

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

These were accidentally removed in rust-lang#94247 because the representation was changed from &mut [T] to *mut T, which has !Send + !Sync.

m-ou-se · 2022-08-03T09:26:45Z

@bors r+

bors · 2022-08-03T09:26:47Z

📌 Commit 22dfbdd has been approved by m-ou-se

It is now in the queue for this repository.

Mark-Simulacrum · 2022-08-03T10:00:14Z

@bors p=1 to push this through before we branch beta

bors · 2022-08-03T13:18:02Z

⌛ Testing commit 22dfbdd with merge 04f72f9...

RalfJung · 2022-08-03T14:01:42Z

library/core/tests/slice.rs

+        RChunksMut<'static, Cell<i32>>: Send,
+        RChunksMut<'static, MutexGuard<'static, u32>>: Sync,
+        RChunksExactMut<'static, Cell<i32>>: Send,
+        RChunksExactMut<'static, MutexGuard<'static, u32>>: Sync,


Is that a reliable test? Don't we allow false where clauses that can never be satisfied (or might allow such things in the future)?

IMO the test should also call this function, to ensure its where clauses can actually be satisfied. An unused function with precondition False proves nothing.

Yeah, we need an instantiation of it (not necessarily a call), à là:

let _ = assert_send_and_sync;

I guess a #[deny(trivial_bounds)] could do the job as well, since even with feature(trivial_bounds) at least that lint gets fired

I may have time to address this in a follow-up tomorrow. I didn't want to delay this PR, and I don't have any spare time for this in the near future.

danielhenrymantilla · 2022-08-03T14:37:42Z

library/core/src/slice/iter.rs

@@ -2114,6 +2120,12 @@ unsafe impl<'a, T> TrustedRandomAccessNoCoerce for ChunksExactMut<'a, T> {
    const MAY_HAVE_SIDE_EFFECT: bool = false;
 }

+#[stable(feature = "chunks_exact", since = "1.31.0")]
+unsafe impl<T> Send for ChunksExactMut<'_, T> where T: Send {}


Debatable nit, but pointing it out just for consideration:

Suggested change

unsafe impl<T> Send for ChunksExactMut<'_, T> where T: Send {}

unsafe impl<'r, T> Send for ChunksExactMut<'r, T> where &'r mut [T]: Send {}

(this makes it "easier" to understand the causality relation, imho)

and so on for the others

bors · 2022-08-03T16:09:50Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 04f72f9 to master...

rust-timer · 2022-08-03T17:24:59Z

Finished benchmarking commit (04f72f9): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.4%	-0.5%	3
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.9%	3.2%	3
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.8%	-3.8%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

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

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

@rustbot label: -perf-regression

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

…-ou-se Fix test: chunks_mut_are_send_and_sync Follow-up to rust-lang#100023 to make the test actually effective

Fix test: chunks_mut_are_send_and_sync Follow-up to rust-lang/rust#100023 to make the test actually effective

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

rust-highfive assigned m-ou-se Aug 1, 2022

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

This comment has been minimized.

Sign in to view

saethlin force-pushed the send-sync-chunksmut branch from 34eed06 to 583b10d Compare August 1, 2022 13:55

This comment has been minimized.

Sign in to view

Add back Send and Sync impls on ChunksMut iterators

22dfbdd

These were accidentally removed in rust-lang#94247 because the representation was changed from &mut [T] to *mut T, which has !Send + !Sync.

saethlin force-pushed the send-sync-chunksmut branch from 583b10d to 22dfbdd Compare August 1, 2022 14:32

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 Aug 3, 2022

RalfJung reviewed Aug 3, 2022

View reviewed changes

danielhenrymantilla reviewed Aug 3, 2022

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2022

bors merged commit 04f72f9 into rust-lang:master Aug 3, 2022

rustbot added this to the 1.64.0 milestone Aug 3, 2022

RalfJung mentioned this pull request Aug 3, 2022

Fix test: chunks_mut_are_send_and_sync #100112

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 11, 2022

Rollup merge of rust-lang#100112 - RalfJung:assert_send_and_sync, r=m…

275d4e7

…-ou-se Fix test: chunks_mut_are_send_and_sync Follow-up to rust-lang#100023 to make the test actually effective

saethlin deleted the send-sync-chunksmut branch September 3, 2022 23:26

workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022

Rollup merge of #100112 - RalfJung:assert_send_and_sync, r=m-ou-se

f1c25c6

Fix test: chunks_mut_are_send_and_sync Follow-up to rust-lang/rust#100023 to make the test actually effective

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back Send and Sync impls on ChunksMut iterators #100023

Add back Send and Sync impls on ChunksMut iterators #100023

saethlin commented Aug 1, 2022

rustbot commented Aug 1, 2022

rust-highfive commented Aug 1, 2022

This comment has been minimized.

This comment has been minimized.

m-ou-se commented Aug 3, 2022

bors commented Aug 3, 2022

Mark-Simulacrum commented Aug 3, 2022

bors commented Aug 3, 2022

RalfJung Aug 3, 2022 •

edited

Loading

danielhenrymantilla Aug 3, 2022 •

edited

Loading

saethlin Aug 3, 2022

RalfJung Aug 3, 2022

danielhenrymantilla Aug 3, 2022 •

edited

Loading

bors commented Aug 3, 2022

rust-timer commented Aug 3, 2022

	unsafe impl<T> Send for ChunksExactMut<'_, T> where T: Send {}
	unsafe impl<'r, T> Send for ChunksExactMut<'r, T> where &'r mut [T]: Send {}

Add back Send and Sync impls on ChunksMut iterators #100023

Add back Send and Sync impls on ChunksMut iterators #100023

Conversation

saethlin commented Aug 1, 2022

rustbot commented Aug 1, 2022

rust-highfive commented Aug 1, 2022

This comment has been minimized.

This comment has been minimized.

m-ou-se commented Aug 3, 2022

bors commented Aug 3, 2022

Mark-Simulacrum commented Aug 3, 2022

bors commented Aug 3, 2022

RalfJung Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

danielhenrymantilla Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

saethlin Aug 3, 2022

Choose a reason for hiding this comment

RalfJung Aug 3, 2022

Choose a reason for hiding this comment

danielhenrymantilla Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

bors commented Aug 3, 2022

rust-timer commented Aug 3, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

RalfJung Aug 3, 2022 •

edited

Loading

danielhenrymantilla Aug 3, 2022 •

edited

Loading

danielhenrymantilla Aug 3, 2022 •

edited

Loading