Messages in this thread | | | From | Andreas Hindborg <> | Subject | Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue | Date | Tue, 30 May 2023 09:19:52 +0200 |
| |
Alice Ryhl <aliceryhl@google.com> writes:
> On 5/18/23 21:17, Martin Rodriguez Reboredo wrote: >> On 5/17/23 17:31, Alice Ryhl wrote: >>> +unsafe impl<T> WorkItem for Arc<T> >>> +where >>> + T: ArcWorkItem + HasWork<Self> + ?Sized, >>> +{ >>> + type EnqueueOutput = Result<(), Self>; >>> + >>> + unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput >>> + where >>> + F: FnOnce(*mut bindings::work_struct) -> bool, >>> + { >>> + let ptr = Arc::into_raw(self); >>> + >>> + // Using `get_work_offset` here for object-safety. >>> + // >>> + // SAFETY: The pointer is valid since we just got it from `into_raw`. >>> + let off = unsafe { (&*ptr).get_work_offset() }; >>> + >>> + // SAFETY: The `HasWork` impl promises that this offset gives us a field of type >>> + // `Work<Self>` in the same allocation. >>> + let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> }; >>> + // SAFETY: The pointer is not dangling. >>> + let work_ptr = unsafe { Work::raw_get(work_ptr) }; >>> + >>> + match (queue_work_on)(work_ptr) { >> >> Match for boolean is not a good pattern in my eyes, if-else should be >> used instead. > > I think this is a question of style. For a comparison: > > match (queue_work_on)(work_ptr) { > true => Ok(()), > // SAFETY: The work queue has not taken ownership of the pointer. > false => Err(unsafe { Arc::from_raw(ptr) }), > } > > vs > > if (queue_work_on)(work_ptr) { > Ok(()) > } else { > // SAFETY: The work queue has not taken ownership of the pointer. > Err(unsafe { Arc::from_raw(ptr) }), > } > > I'm happy to change it if others disagree, but when the branches > evaluate to a short expression like they do here, I quite like the first > version.
I prefer the first one, but both look OK to me. Is one more idiomatic than the other, or is it just a matter of personal preference?
BR Andreas
> >> Also aren't the parens around the closure unnecessary? > > Hmm, parenthesises are often required around closures, but it's possible > that it is only required for stuff like `self.closure(args)` to > disambiguate between a `closure` field (of pointer type) and a `closure` > method. I can check and remove them if they are not necessary.
| |