Re: [PATCH v4] rust: add global lock support

[Boqun Feng]

On Thu, Oct 10, 2024 at 10:21:41PM +0000, Benno Lossin wrote:
> On 10.10.24 18:33, Boqun Feng wrote:
> > On Thu, Oct 10, 2024 at 07:29:32AM -0700, Boqun Feng wrote:
> >> On Thu, Oct 10, 2024 at 03:58:07PM +0200, Alice Ryhl wrote:
> >>> On Thu, Oct 10, 2024 at 3:55 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> >>>>
> >>>> On Thu, Oct 10, 2024 at 12:53:00PM +0200, Alice Ryhl wrote:
> >>>> [...]
> >>>>>>> +#[macro_export]
> >>>>>>> +macro_rules! global_lock {
> >>>>>>> +    {
> >>>>>>> +        $(#[$meta:meta])* $pub:vis static $name:ident: $kind:ident<$valuety:ty> = unsafe { uninit };
> >>>>>>> +        value: $value:expr;
> >>>>>>
> >>>>>> I would find it more natural to use `=` instead of `:` here, since then
> >>>>>> it would read as a normal statement with the semicolon at the end.
> >>>>>> Another alternative would be to use `,` instead of `;`, but that doesn't
> >>>>>> work nicely with the static keyword above (although you could make the
> >>>>>> user write it in another {}, but that also isn't ideal...).
> >>>>>>
> >>>>>> Using `=` instead of `:` makes my editor put the correct amount of
> >>>>>> indentation there, `:` adds a lot of extra spaces.
> >>>>>
> >>>>> That seems sensible.
> >>>>>
> >>>>
> >>>> While we are at it, how about we make the syntax:
> >>>>
> >>>>         global_lock!{
> >>>>             static MY_LOCK: Mutex<u32> = unsafe { 0 };
> >>>>         }
> >>>>
> >>>> or
> >>>>
> >>>>         global_lock!{
> >>>>             static MY_LOCK: Mutex<u32> = unsafe { uninit { 0 } };
> >>>>         }
> >>>>
> >>>> ?
> >>>>
> >>>> i.e. instead of a "value" field, we put it in the "initialization
> >>>> expression". To me, this make it more clear that "value" is the
> >>>> initialized value protected by the lock. Thoughts?
> >>>
> >>> `uninit { 0 }` looks pretty terrible IMO. Can we come up with something better?
> >>>
> >>
> > 
> > how about:
> > 
> >         global_lock!{
> >             static MY_LOCK: Mutex<u32> = unsafe { data: 0 };
> 
> I dislike this, since there is no `uninit` anywhere, but the mutex needs
> to be initialized.
> 
> >         }
> > 
> > ?
> > 
> > "data: " will make it clear that the value is not for the lock state.
> > "uninit" is dropped because the "unsafe" already requires the global
> > variable to be initialised first. Or "unsafe { uninit, data: 0 }" if you
> > want to keep the "uninit" part?
> 
> That also looks weird to me...
> 
> But I haven't come up with a good alternative
> 

How about a "fake" MaybyUninit:

	global_lock!{
            static MY_LOCK: Mutex<u32> = unsafe { MaybeUninit::new(0).assume_init() };
	}

?

I feel like we need to put the data in the initialization expression
because if we resolve the initialization issues and can skip the extra
init step, we pretty much want to use the macro like:

	global_lock!{
            static MY_LOCK: Mutex<u32> = { data: 0 };
	    // maybe even
            // static MY_LOCK: Mutex<u32> = { 0 };
	}

instead of

	global_lock!{
            static MY_LOCK: Mutex<u32> = init;
	    value = 0;
	}

, right?

So we need to think about providing a smooth way for users to transfer.
Not just adjust the changes (which I believe is a good practice for
coccinelle), but also the conceptual model "oh now I don't need to
provide a 'value=' field?". Hence even though the above proposals may
look weird, but I think that's still better?

Regards,
Boqun


> ---
> Cheers,
> Benno
>