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

[Benno Lossin]

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

---
Cheers,
Benno