Messages in this thread | | | From | Eric Dumazet <> | Date | Tue, 2 Apr 2024 14:45:07 +0200 | Subject | Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow warnings throughout the file |
| |
On Tue, Apr 2, 2024 at 1:55 PM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Jakub Kicinski <kuba@kernel.org> > Date: Fri, 29 Mar 2024 13:53:44 -0700 > > > On Fri, 29 Mar 2024 13:18:57 -0700 Jakub Kicinski wrote: > >>> Sparse: > >>> > >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: warning: symbol '_res' shadows an earlier one > >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: originally declared here > >> > >> I don't see these building with LLVM=1 W=12 C=1 > >> and I really don't like complicating the code because the compiler > >> is stupid. Can't you solve this with some renames? Add another > > It's not the compiler, its warnings are valid actually. Shadowing makes > it very easy to confuse variables and make bugs... > > >> underscore or something? > > > > I'm stupid I tried on the test branch which already had your fix.. > > :D Sometimes it happens. > > > > > This is enough: > > > > diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h > > index 1ec408585373..2270fbb99cf7 100644 > > --- a/include/net/netdev_queues.h > > +++ b/include/net/netdev_queues.h > > @@ -89,7 +89,7 @@ struct netdev_stat_ops { > > > > #define netif_txq_try_stop(txq, get_desc, start_thrs) \ > > ({ \ > > - int _res; \ > > + int __res; \ > > \ > > netif_tx_stop_queue(txq); \ > > /* Producer index and stop bit must be visible \ > > @@ -101,12 +101,12 @@ struct netdev_stat_ops { > > /* We need to check again in a case another \ > > * CPU has just made room available. \ > > */ \ > > - _res = 0; \ > > + __res = 0; \ > > if (unlikely(get_desc >= start_thrs)) { \ > > netif_tx_start_queue(txq); \ > > - _res = -1; \ > > + __res = -1; \ > > } \ > > - _res; \ > > + __res; \ > > }) \ > > > > /** > > But what if there's a function which calls one of these functions and > already has _res or __res or something? I know renaming is enough for > the warnings I mentioned, but without __UNIQUE_ID() anything can happen > anytime, so I wanted to fix that once and for all :z > > I already saw some macros which have a layer of indirection for > __UNIQUE_ID(), but previously they didn't and then there were fixes > which added underscores, renamed variables etc etc... >
We have hundreds of macros in include/ directory which use local names without __UNIQUE_ID()
What is the plan ? Hundreds of patches obfuscating them more than they are ?
Can you show us how rb_entry_safe() (random choice) would be rewritten ?
| |