| Message ID | 20231217-i40e-comma-v1-1-85c075eff237@kernel.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-next] i40e: Avoid unnecessary use of comma operator | expand |
On Sun, Dec 17, 2023 at 1:45 AM Simon Horman <horms@kernel.org> wrote: > > Although it does not seem to have any untoward side-effects, > the use of ';' to separate to assignments seems more appropriate than ','. > > Flagged by clang-17 -Wcomma Yikes! This kind of example is why I hate the comma operator! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> (Is -Wcomma enabled by -Wall?) Is there a fixes tag we can add? > > No functional change intended. > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > index 812d04747bd0..f542f2671957 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > @@ -1917,7 +1917,7 @@ int i40e_get_eeprom(struct net_device *netdev, > len = eeprom->len - (I40E_NVM_SECTOR_SIZE * i); > last = true; > } > - offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i), > + offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i); > ret_val = i40e_aq_read_nvm(hw, 0x0, offset, len, > (u8 *)eeprom_buff + (I40E_NVM_SECTOR_SIZE * i), > last, NULL); > >
On Mon, Dec 18, 2023 at 08:32:28AM -0800, Nick Desaulniers wrote: > On Sun, Dec 17, 2023 at 1:45 AM Simon Horman <horms@kernel.org> wrote: > > > > Although it does not seem to have any untoward side-effects, > > the use of ';' to separate to assignments seems more appropriate than ','. > > > > Flagged by clang-17 -Wcomma > > Yikes! This kind of example is why I hate the comma operator! > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > (Is -Wcomma enabled by -Wall?) No and last time that I looked into enabling it, there were a lot of instances in the kernel: https://lore.kernel.org/20230630192825.GA2745548@dev-arch.thelio-3990X/ It is still probably worth pursuing at some point but that is a lot of instances to clean up (along with potentially having a decent amount of pushback depending on the changes necessary to eliminate all instances). > Is there a fixes tag we can add? > > > > > No functional change intended. > > Compile tested only. > > > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > index 812d04747bd0..f542f2671957 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > @@ -1917,7 +1917,7 @@ int i40e_get_eeprom(struct net_device *netdev, > > len = eeprom->len - (I40E_NVM_SECTOR_SIZE * i); > > last = true; > > } > > - offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i), > > + offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i); > > ret_val = i40e_aq_read_nvm(hw, 0x0, offset, len, > > (u8 *)eeprom_buff + (I40E_NVM_SECTOR_SIZE * i), > > last, NULL); > > > > > > > -- > Thanks, > ~Nick Desaulniers >
On Mon, Dec 18, 2023 at 11:00 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Mon, Dec 18, 2023 at 08:32:28AM -0800, Nick Desaulniers wrote: > > (Is -Wcomma enabled by -Wall?) > > No and last time that I looked into enabling it, there were a lot of > instances in the kernel: > > https://lore.kernel.org/20230630192825.GA2745548@dev-arch.thelio-3990X/ > > It is still probably worth pursuing at some point but that is a lot of > instances to clean up (along with potentially having a decent amount of > pushback depending on the changes necessary to eliminate all instances). Filed this todo: https://github.com/ClangBuiltLinux/linux/issues/1968 I'd be happy if Simon keeps poking at getting that warning enabled.
On Mon, Dec 18, 2023 at 08:32:28AM -0800, Nick Desaulniers wrote: > On Sun, Dec 17, 2023 at 1:45 AM Simon Horman <horms@kernel.org> wrote: > > > > Although it does not seem to have any untoward side-effects, > > the use of ';' to separate to assignments seems more appropriate than ','. > > > > Flagged by clang-17 -Wcomma > > Yikes! This kind of example is why I hate the comma operator! > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > (Is -Wcomma enabled by -Wall?) > > Is there a fixes tag we can add? Hi Nick, I don't believe this resolves a user-visible bug, so for Networking code a fixes tag isn't appropriate. > > > > > No functional change intended. > > Compile tested only. > > > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > index 812d04747bd0..f542f2671957 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > @@ -1917,7 +1917,7 @@ int i40e_get_eeprom(struct net_device *netdev, > > len = eeprom->len - (I40E_NVM_SECTOR_SIZE * i); > > last = true; > > } > > - offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i), > > + offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i); > > ret_val = i40e_aq_read_nvm(hw, 0x0, offset, len, > > (u8 *)eeprom_buff + (I40E_NVM_SECTOR_SIZE * i), > > last, NULL); > > > > > > > -- > Thanks, > ~Nick Desaulniers
On Mon, Dec 18, 2023 at 11:08:38AM -0800, Nick Desaulniers wrote: > On Mon, Dec 18, 2023 at 11:00 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Mon, Dec 18, 2023 at 08:32:28AM -0800, Nick Desaulniers wrote: > > > (Is -Wcomma enabled by -Wall?) > > > > No and last time that I looked into enabling it, there were a lot of > > instances in the kernel: > > > > https://lore.kernel.org/20230630192825.GA2745548@dev-arch.thelio-3990X/ > > > > It is still probably worth pursuing at some point but that is a lot of > > instances to clean up (along with potentially having a decent amount of > > pushback depending on the changes necessary to eliminate all instances). > > Filed this todo: > https://github.com/ClangBuiltLinux/linux/issues/1968 > I'd be happy if Simon keeps poking at getting that warning enabled. FWIIW, since the discussion cited above I have been keeping an eye on -Wcomma, mostly wrt to patches for Networking code. My subjective feelings on this are: * Few new instances seem to be added * There are some, though I wouldn't say a lot, of existing instances in files that are that is being updated. * I don't recall any of the instances, new or old, being bugs. Though perhaps a very small number were. So while I'm all for more checks. And I'm all for only using the comma where it is necessary (I suspect that often it is a typo). I do not get the feeling that we are sitting on a trove of nasty bugs.
On Tue, Dec 19, 2023 at 2:12 AM Simon Horman <horms@kernel.org> wrote: > > So while I'm all for more checks. > And I'm all for only using the comma where it is necessary > (I suspect that often it is a typo). > I do not get the feeling that we are sitting on a trove of nasty bugs. I still get nightmares frome: - commit e7140639b1de ("powerpc/xmon: Fix opcode being uninitialized in print_insn_powerpc") - commit f7019b7b0ad1 ("xsk: Properly terminate assignment in xskq_produce_flush_desc")
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Simon Horman > Sent: Sunday, December 17, 2023 3:15 PM > To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Cc: netdev@vger.kernel.org; llvm@lists.linux.dev; Nick Desaulniers <ndesaulniers@google.com>; Nathan Chancellor <nathan@kernel.org>; Eric Dumazet <edumazet@google.com>; intel-wired-lan@lists.osuosl.org; Bill Wendling <morbo@google.com>; Justin Stitt <justinstitt@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net> > Subject: [Intel-wired-lan] [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator > > Although it does not seem to have any untoward side-effects, > the use of ';' to separate to assignments seems more appropriate than ','. > > Flagged by clang-17 -Wcomma > > No functional change intended. > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index 812d04747bd0..f542f2671957 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -1917,7 +1917,7 @@ int i40e_get_eeprom(struct net_device *netdev, len = eeprom->len - (I40E_NVM_SECTOR_SIZE * i); last = true; } - offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i), + offset = eeprom->offset + (I40E_NVM_SECTOR_SIZE * i); ret_val = i40e_aq_read_nvm(hw, 0x0, offset, len, (u8 *)eeprom_buff + (I40E_NVM_SECTOR_SIZE * i), last, NULL);
Although it does not seem to have any untoward side-effects, the use of ';' to separate to assignments seems more appropriate than ','. Flagged by clang-17 -Wcomma No functional change intended. Compile tested only. Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)