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

[Vincent Mailhol <mailhol@kernel.org>]

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?

> So the patch won't really fix the paranoid warning.
> I think it's better to fix sparse to silence this warn.

Well, it is not only about the tooling but also about the Linux kernel coding
style. I quote:

  5) namespace collisions when defining local variables in macros resembling
  functions:

	#define FOO(x)                          \
	({                                      \
	        typeof(x) ret;                  \
	        ret = calc_ret(x);              \
	        (ret);                          \
	})

  ret is a common name for a local variable - __foo_ret is less likely to
  collide with an existing variable.

I really do not want to open the debate on whether shadow warnings are useful or
not. The coding style says that we should care about this, let's just follow the
rule.

Your patch is causing noise in the files I am using. So I hope you can find a
solution for the annoyance your are causing me. I do not mind which solution it
is. I am proposing one, if you do not like it, I am also fine if you prefer to
go on a quest to rewrite the coding style and modify the tooling so that we do
not warn anymore about shadowing. But I think we can agree that it is not *my*
job to rewrite the kernel coding style and the tooling to *your* liking.


Yours sincerely,
Vincent Mailhol