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

Submitted by santosh nayak on Feb. 24, 2012, 4:57 p.m.

Details

Message ID 1330102654-2474-1-git-send-email-santoshprasadnayak@gmail.com
State Rejected
Delegated to: David Miller
Headers show

Commit Message

santosh nayak Feb. 24, 2012, 4:57 p.m.
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(-)

Comments

walter harms Feb. 24, 2012, 5:27 p.m.
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,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Feb. 24, 2012, 7:10 p.m.
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,
David Miller Feb. 24, 2012, 10:50 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

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,