Re: [PATCH] locking/local_lock: fix shadowing in __local_lock_acquire()

[Vincent Mailhol <mailhol@kernel.org>]

On 25/09/2025 at 02:07, Alexei Starovoitov wrote:
> On Wed, Sep 24, 2025 at 4:07 PM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> On 24/09/2025 at 19:26, Vincent Mailhol wrote:
>>> On 24/09/2025 at 04:38, Alexei Starovoitov wrote:
>>>> On Tue, Sep 23, 2025 at 7:02 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>>>>>
>>>>> The __local_lock_acquire() macro uses a local variable named 'l'. This
>>>>> being a common name, there is a risk of shadowing other variables.
>>>>>
>>>>> For example, it is currently shadowing the parameter 'l' of the:
>>>>>
>>>>>   class_##_name##_t class_##_name##_constructor(_type *l)
>>>>>
>>>>> function factory from linux/cleanup.h.
>>>>>
>>>>> Both sparse (with default options) and GCC (with W=2 option) warn
>>>>> about this shadowing.
>>>>>
>>>>> This is a bening warning, but because the issue appears in a header,
>>>>> it is spamming whoever is using it. So better to fix to remove some
>>>>> noise.
>>>>>
>>>>> Rename the variable from 'l' to '__lock' (with two underscore prefixes
>>>>> as suggested in the Linux kernel coding style [1]) in order to prevent
>>>>> the name collision.
>>>>
>>>> lockdep has __lock as a local variable.
>>>
>>> OK. I didn't saw this one.
>>>
>>> But there is a major difference between a shadowing in lockdep.c versus a
>>> shadowing in an header: the shadowing warning is local to lockdep.c and does not
>>> pollute the other users.
>>>
>>> My worry is only about the spam created by warnings in headers.
>>>
>>> Regardless, would renaming to __locked or __l instead of __lock help to address
>>> your concern?
>>
>> __locked was a bad suggestion. It could be named __local_lock (there is already
>> a __local_lock() function like macro, but function like macro does not conflict
>> with variable names).
>>
>> But now, my current preference goes to __ll (and also, to keep things
>> consistent, __tl for the trylock).
> 
> I think s/l/__l/ and s/tl/__tl/ is fine,
> but do it for all macros in that file,
> since renaming one is fishy. It doesn't fix what
> you're claiming to fix, hence the pushback.

OK. This was a fair pushback :)

I took your suggestion and renamed all instances in the file. I just sent the v2.

Thanks!


Yours sincerely,
Vincent Mailhol