Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support
From: Andreas Hindborg
Date: Thu Sep 19 2024 - 02:02:20 EST
Hi Benno,
Thanks for the feedback. I will incorporate all the whitespace
suggestions you have =F0=9F=91=8D
"Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
> On 18.09.24 00:27, Andreas Hindborg wrote:
[...]
>> +
>> +impl<T> Timer<T> {
>> + /// Return an initializer for a new timer instance.
>> + pub fn new() -> impl PinInit<Self>
>> + where
>> + T: TimerCallback,
>> + {
>> + pin_init!( Self {
>
> I would remove the space after the `(`.
> Would be great if we had rustfmt support for custom macros.
Yes, that would be great!
>
>> + // INVARIANTS: We initialize `timer` with `hrtimer_init` be=
low.
>> + timer <- Opaque::ffi_init(move |place: *mut bindings::hrtim=
er| {
>> + // SAFETY: By design of `pin_init!`, `place` is a point=
er live
>> + // allocation. hrtimer_init will initialize `place` and=
does not
>> + // require `place` to be initialized prior to the call.
>> + unsafe {
>> + bindings::hrtimer_init(
>> + place,
>> + bindings::CLOCK_MONOTONIC as i32,
>> + bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> + );
>> + }
>> +
>> + // SAFETY: `place` is pointing to a live allocation, so=
the deref
>> + // is safe.
>> + let function: *mut Option<_> =3D
>
> Do you really need this type hint?
Apparently not!
[...]
>> +pub trait TimerPointer: Sync + Sized {
>> + /// A handle representing a scheduled timer.
>> + ///
>> + /// If the timer is armed or if the timer callback is running when =
the
>> + /// handle is dropped, the drop method of `TimerHandle` should not =
return
>> + /// until the timer is unarmed and the callback has completed.
>> + ///
>> + /// Note: It must be safe to leak the handle.
>> + type TimerHandle: TimerHandle;
>
> Why does this need to be an associated type? Couldn't we have a
> `TimerHandle<T>` struct? The schedule function below could then return
> `TimerHandle<Self>`.
At one point, I had some cycles in trait resolution. Moving generics to
associated types solved that issue. Maybe this can be changed to a
generic. But are generics preferred over associated types for some
reason?
Best regards,
Andreas