Skip to content

Commit 58f7b7e

Browse files
committedFeb 27, 2023
Be careful comparing job_ref.execute_fn
After rayon-rs#1025, clippy is denying [`fn_address_comparisons`][1], and this is correct that we can't actually guarantee that the pointer we got from `as_job_ref` will be the same that we produce in a later `PartialEq`. It's less worrying that different functions could merge to the same address, because our `StackJob` will still have different data pointers. We can keep `Copy + Eq` off of `JobRef` with an opaque `id() -> impl Eq` instead, so we'll be comparing the exact same captured `fn` pointer. [1]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_address_comparisons
1 parent e1c2aad commit 58f7b7e

File tree

2 files changed

+11
-13
lines changed

2 files changed

+11
-13
lines changed
 

‎rayon-core/src/job.rs

+7-11
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,13 @@ impl JobRef {
5252
}
5353
}
5454

55+
/// Returns an opaque handle that can be saved and compared,
56+
/// without making `JobRef` itself `Copy + Eq`.
57+
#[inline]
58+
pub(super) fn id(&self) -> impl Eq {
59+
(self.pointer, self.execute_fn)
60+
}
61+
5562
#[inline]
5663
pub(super) unsafe fn execute(self) {
5764
(self.execute_fn)(self.pointer)
@@ -73,17 +80,6 @@ where
7380
result: UnsafeCell<JobResult<R>>,
7481
}
7582

76-
impl<L, F, R> PartialEq<JobRef> for StackJob<L, F, R>
77-
where
78-
L: Latch + Sync,
79-
F: FnOnce(bool) -> R + Send,
80-
R: Send,
81-
{
82-
fn eq(&self, job: &JobRef) -> bool {
83-
job.pointer == <*const Self>::cast(self) && job.execute_fn == Self::execute
84-
}
85-
}
86-
8783
impl<L, F, R> StackJob<L, F, R>
8884
where
8985
L: Latch + Sync,

‎rayon-core/src/join/mod.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,9 @@ where
134134
// done here so that the stack frame can keep it all live
135135
// long enough.
136136
let job_b = StackJob::new(call_b(oper_b), SpinLatch::new(worker_thread));
137-
worker_thread.push(job_b.as_job_ref());
137+
let job_b_ref = job_b.as_job_ref();
138+
let job_b_id = job_b_ref.id();
139+
worker_thread.push(job_b_ref);
138140

139141
// Execute task a; hopefully b gets stolen in the meantime.
140142
let status_a = unwind::halt_unwinding(call_a(oper_a, injected));
@@ -150,7 +152,7 @@ where
150152
// those off to get to it.
151153
while !job_b.latch.probe() {
152154
if let Some(job) = worker_thread.take_local_job() {
153-
if job_b == job {
155+
if job_b_id == job.id() {
154156
// Found it! Let's run it.
155157
//
156158
// Note that this could panic, but it's ok if we unwind here.

0 commit comments

Comments
 (0)
Please sign in to comment.