[PATCH 2/2] enic: Silence Sparse Warning: "dubious: x | !y"

[PATCH 2/2] enic: Silence Sparse Warning: "dubious: x | !y"

[santosh nayak]

From: Santosh Nayak <santoshprasadnayak@gmail.com>

Sparse is giving the following warning:
"warning: dubious: x | !y"

"enic_are_pp_different" is static and expected to return true or false.
Logical or is indended here. With logical or, in best case, execution will be
faster because if leftmost operand is true then no need to check other operands.

Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
 drivers/net/ethernet/cisco/enic/enic_pp.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_pp.c b/drivers/net/ethernet/cisco/enic/enic_pp.c
index dafea1e..3a6de22 100644
--- a/drivers/net/ethernet/cisco/enic/enic_pp.c
+++ b/drivers/net/ethernet/cisco/enic/enic_pp.c
@@ -159,10 +159,10 @@ static int enic_unset_port_profile(struct enic *enic, int vf)
 static int enic_are_pp_different(struct enic_port_profile *pp1,
 		struct enic_port_profile *pp2)
 {
-	return strcmp(pp1->name, pp2->name) | !!memcmp(pp1->instance_uuid,
-		pp2->instance_uuid, PORT_UUID_MAX) |
-		!!memcmp(pp1->host_uuid, pp2->host_uuid, PORT_UUID_MAX) |
-		!!memcmp(pp1->mac_addr, pp2->mac_addr, ETH_ALEN);
+	return strcmp(pp1->name, pp2->name) || !!memcmp(pp1->instance_uuid,
+		pp2->instance_uuid, PORT_UUID_MAX) ||
+		!!memcmp(pp1->host_uuid, pp2->host_uuid, PORT_UUID_MAX) ||
+		!!memcmp(pp1->mac_addr, pp2->mac_addr, ETH_ALEN);
 }
 
 static int enic_pp_preassociate(struct enic *enic, int vf,
-- 
1.7.4.4

Re: [PATCH 2/2] enic: Silence Sparse Warning: "dubious: x | !y"

[walter harms]

Am 24.02.2012 17:57, schrieb santosh nayak:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> Sparse is giving the following warning:
> "warning: dubious: x | !y"
> 
> "enic_are_pp_different" is static and expected to return true or false.
> Logical or is indended here. With logical or, in best case, execution will be
> faster because if leftmost operand is true then no need to check other operands.
> 
> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic_pp.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_pp.c b/drivers/net/ethernet/cisco/enic/enic_pp.c
> index dafea1e..3a6de22 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_pp.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_pp.c
> @@ -159,10 +159,10 @@ static int enic_unset_port_profile(struct enic *enic, int vf)
>  static int enic_are_pp_different(struct enic_port_profile *pp1,
>  		struct enic_port_profile *pp2)
>  {
> -	return strcmp(pp1->name, pp2->name) | 
                !!memcmp(pp1->instance_uuid,pp2->instance_uuid, PORT_UUID_MAX) |
> -		!!memcmp(pp1->host_uuid, pp2->host_uuid, PORT_UUID_MAX) |
> -		!!memcmp(pp1->mac_addr, pp2->mac_addr, ETH_ALEN);
> +	return  strcmp(pp1->name, pp2->name) || 
	        !!memcmp(pp1->instance_uuid,pp2->instance_uuid, PORT_UUID_MAX) ||
> +		!!memcmp(pp1->host_uuid, pp2->host_uuid, PORT_UUID_MAX) ||
> +		!!memcmp(pp1->mac_addr, pp2->mac_addr, ETH_ALEN);
>  }
>  

to be fair, this is next to unreadable (not your fault of cause).
Instead of squeezing this into a one statement i would suggest more statements.

just my 2 cents,
re,
 wh



>  static int enic_pp_preassociate(struct enic *enic, int vf,

Re: [PATCH 2/2] enic: Silence Sparse Warning: "dubious: x | !y"

[Ben Hutchings]

On Fri, 2012-02-24 at 22:27 +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> Sparse is giving the following warning:
> "warning: dubious: x | !y"
> 
> "enic_are_pp_different" is static and expected to return true or false.
> Logical or is indended here. With logical or, in best case, execution will be
> faster because if leftmost operand is true then no need to check other operands.

The shortcut behaviour of logical-or means more conditional jumps to be
predicted and more potential for pipeline stalls.  So bitwise-or can in
some cases be more efficient.  But this function appears to be
control-path code where micro-optimisation just doesn't matter.  Use of
logical-or can be justified purely on the grounds of clarity.

Ben.

> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic_pp.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_pp.c b/drivers/net/ethernet/cisco/enic/enic_pp.c
> index dafea1e..3a6de22 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_pp.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_pp.c
> @@ -159,10 +159,10 @@ static int enic_unset_port_profile(struct enic *enic, int vf)
>  static int enic_are_pp_different(struct enic_port_profile *pp1,
>  		struct enic_port_profile *pp2)
>  {
> -	return strcmp(pp1->name, pp2->name) | !!memcmp(pp1->instance_uuid,
> -		pp2->instance_uuid, PORT_UUID_MAX) |
> -		!!memcmp(pp1->host_uuid, pp2->host_uuid, PORT_UUID_MAX) |
> -		!!memcmp(pp1->mac_addr, pp2->mac_addr, ETH_ALEN);
> +	return strcmp(pp1->name, pp2->name) || !!memcmp(pp1->instance_uuid,
> +		pp2->instance_uuid, PORT_UUID_MAX) ||
> +		!!memcmp(pp1->host_uuid, pp2->host_uuid, PORT_UUID_MAX) ||
> +		!!memcmp(pp1->mac_addr, pp2->mac_addr, ETH_ALEN);
>  }
>  
>  static int enic_pp_preassociate(struct enic *enic, int vf,

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 2/2] enic: Silence Sparse Warning: "dubious: x | !y"

[David Miller]

From: santosh nayak <santoshprasadnayak@gmail.com>
Date: Fri, 24 Feb 2012 22:27:34 +0530

> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> Sparse is giving the following warning:
> "warning: dubious: x | !y"
> 
> "enic_are_pp_different" is static and expected to return true or false.
> Logical or is indended here. With logical or, in best case, execution will be
> faster because if leftmost operand is true then no need to check other operands.
> 
> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>

As others have explained, this code is intentional.  It's faster because
the resulting code has less branches and therefore less potential
branch mispredictions.