Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Vlastimil Babka <vbabka@suse.cz>]

On 9/27/25 05:04, Vincent Mailhol wrote:
> On 9/26/25 11:16 PM, Vlastimil Babka wrote:
>> +CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
>> added to the section, should we?
>> 
>> On 9/24/25 20:03, Vincent Mailhol wrote:
>>> The Linux kernel coding style [1] advises to avoid common variable
>>> names in function-like macros to reduce the risk of collisions.
>> 
>> I think it would be better if the tools like sparse could recognize if the
>> shadowing happens inside a macro only and thus really unlikely to cause a
>> misuse due to confusion (code thinks it's manipulating an outer instance but
>> instead it's the inner one), because macros in their definition would never
>> intend to manipulate a possible outer instance, right? Or are there any
>> other problems due to shadowing besides this risk?
> 
> Thank would mean:
> 
>   - rewriting the shadowing check in sparse
>   - removing the -Wshadow from the W=2 list
>   - modifying the kernel coding style
> 
> I am not against this. But I am not unhappy with the current status quo either.
> 
> So far, I kept sending patches whenever I saw such shadow warning in header
> files. And over the last five years, this resulted in only three occurrences:
> 
>   - commit 146034fed6ee ("x86/asm/bitops: Use __builtin_ffs() to evaluate
>     constant expressions")
>     Link: https://git.kernel.org/torvalds/c/146034fed6ee
> 
> 
>   - commit 9ce02f0fc683 ("x86/bug: Prevent shadowing in __WARN_FLAGS")
>     Link: https://git.kernel.org/torvalds/c/9ce02f0fc683
> 
>   - this patch
> 
> Between sending one patch every couple year or enrolling to a quest to modify
> the tooling, my choice is already made. If someone else want to do this change,
> I would be supportive, but that person will not be me.

Thanks for that perspective, with that it seems now clear to me that a rare
fixup of some macro is indeed much easier.

> On a side note, I want to highlight that it is not that I am reluctant to modify
> the tooling. For example, I sent contributed this commit to sparse last week:
> 
> commit 366ad4b2fa3e ("Warn about "unsigned value that used to be signed against
> zero"")
> 
> Link:
> https://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git/commit/?id=366ad4b2fa3e