[PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Simon Horman]

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(-)

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);

Re: [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Nick Desaulniers]

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);
>
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Nathan Chancellor]

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
> 

Re: [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[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.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Simon Horman]

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

Re: [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Simon Horman]

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.

Re: [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Nick Desaulniers]

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")

-- 
Thanks,
~Nick Desaulniers

RE: [Intel-wired-lan] [PATCH iwl-next] i40e: Avoid unnecessary use of comma operator

[Pucha, HimasekharX Reddy]

> -----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)