[PATCH net-next] ieee802154: fix variable declaration and initializer

[PATCH net-next] ieee802154: fix variable declaration and initializer

[Jean Sacren]

The commit 84dda3c648fd ("ieee802154: add support for
listen-before-talk in wpan_phy") introduced the new function
phy_set_lbt() with some minor issues in declaration and
initialization:

1) Fix the variable declaration by changing the type specifier to
   bool as that is the argument type when phy->set_lbt() is called.

2) Fix the initializer by deleting the double logical negation
   operators as they don't serve any purpose.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
Cc: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
---
 net/ieee802154/nl-phy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
index 222310a07762..643dff07803b 100644
--- a/net/ieee802154/nl-phy.c
+++ b/net/ieee802154/nl-phy.c
@@ -379,9 +379,10 @@ static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info)
 
 static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
 {
-	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
+	bool on;
 	int rc;
 
+	on = nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
 	rc = phy->set_lbt(phy, on);
 	if (rc < 0)
 		return rc;

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Eric Dumazet]

On Tue, 2014-03-18 at 21:19 -0600, Jean Sacren wrote:

> 2) Fix the initializer by deleting the double logical negation
>    operators as they don't serve any purpose.
> 
...
>  
>  static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
>  {
> -	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);

You do realize !!(a) is not equivalent to (a) ?





------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Joe Perches]

On Tue, 2014-03-18 at 20:32 -0700, Eric Dumazet wrote:
> On Tue, 2014-03-18 at 21:19 -0600, Jean Sacren wrote:
> 
> > 2) Fix the initializer by deleting the double logical negation
> >    operators as they don't serve any purpose.
> > 
> ...
> >  
> >  static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
> >  {
> > -	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
> 
> You do realize !!(a) is not equivalent to (a) ?

It is when the type it's assigned to also changes
from u8 to bool.

I don't think it's a great style though.
I think the !! doesn't hurt here.

I'd've preferred it to be

	bool on = nla_get_u8(...)

rather than separating the declaration from the assignment
by a few lines of code.

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Eric Dumazet]

On Tue, 2014-03-18 at 20:42 -0700, Joe Perches wrote:
> On Tue, 2014-03-18 at 20:32 -0700, Eric Dumazet wrote:
> > On Tue, 2014-03-18 at 21:19 -0600, Jean Sacren wrote:
> > 
> > > 2) Fix the initializer by deleting the double logical negation
> > >    operators as they don't serve any purpose.
> > > 
> > ...
> > >  
> > >  static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
> > >  {
> > > -	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
> > 
> > You do realize !!(a) is not equivalent to (a) ?
> 
> It is when the type it's assigned to also changes
> from u8 to bool.

I was referring to the changelog, obviously, see how I carefully
copy/pasted the relevant part ?

Stating it is a 'fix' is quite a false statement, I see no fix at all,
maybe a cleanup, but I am not really convinced.

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Jean Sacren]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 18 Mar 2014 21:35:54 -0700
>
> On Tue, 2014-03-18 at 20:42 -0700, Joe Perches wrote:
> > On Tue, 2014-03-18 at 20:32 -0700, Eric Dumazet wrote:
> > > On Tue, 2014-03-18 at 21:19 -0600, Jean Sacren wrote:
> > > 
> > > > 2) Fix the initializer by deleting the double logical negation
> > > >    operators as they don't serve any purpose.
> > > > 
> > > ...
> > > >  
> > > >  static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
> > > >  {
> > > > -	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
> > > 
> > > You do realize !!(a) is not equivalent to (a) ?
> > 
> > It is when the type it's assigned to also changes
> > from u8 to bool.
> 
> I was referring to the changelog, obviously, see how I carefully
> copy/pasted the relevant part ?
> 
> Stating it is a 'fix' is quite a false statement, I see no fix at all,
> maybe a cleanup, but I am not really convinced.

Stating it is a "fix" is not a false statement at all. !! falsely
changes the value to be int TWICE for nothing! Are you still not
convinced?

-- 
Jean Sacren

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Joe Perches]

On Tue, 2014-03-18 at 22:56 -0600, Jean Sacren wrote:
> Stating it is a "fix" is not a false statement at all. !! falsely
> changes the value to be int TWICE for nothing! Are you still not
> convinced?

Not so fast Jean.  There's no logic change.

Changing the type to bool does exactly the same thing
and doesn't change the generated object code at all.

$ diff net/ieee802154/nl-phy.lst.new net/ieee802154/nl-phy.lst.old
2289d2288
< static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
2291c2290
< 	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
---
> 	bool on;
2293a2293
> 	on = nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
2296,2298d2295
< 	return 0;
< }
< 
2301,2303c2298
< 	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
<     1096:	80 78 04 00          	cmpb   $0x0,0x4(%rax)
<     109a:	41 0f 95 c5          	setne  %r13b
---
> 	bool on;
2305a2301,2303
> 	on = nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
>     1096:	80 78 04 00          	cmpb   $0x0,0x4(%rax)
>     109a:	41 0f 95 c5          	setne  %r13b
2322c2320
< 
---



------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Eric Dumazet]

On Tue, 2014-03-18 at 22:56 -0600, Jean Sacren wrote:
> Stating it is a "fix" is not a false statement at all. !! falsely
> changes the value to be int TWICE for nothing! Are you still not
> convinced?

I think you are mistaken.

X = !!(a) makes sure X is 0 or 1.

Its a valid and useful construct.

There is no _bug_ here.

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Joe Perches]

On Tue, 2014-03-18 at 21:35 -0700, Eric Dumazet wrote:
> On Tue, 2014-03-18 at 20:42 -0700, Joe Perches wrote:
> > On Tue, 2014-03-18 at 20:32 -0700, Eric Dumazet wrote:
> > > On Tue, 2014-03-18 at 21:19 -0600, Jean Sacren wrote:
> > > 
> > > > 2) Fix the initializer by deleting the double logical negation
> > > >    operators as they don't serve any purpose.
> > > > 
> > > ...
> > > >  
> > > >  static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
> > > >  {
> > > > -	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
> > > 
> > > You do realize !!(a) is not equivalent to (a) ?
> > 
> > It is when the type it's assigned to also changes
> > from u8 to bool.
> 
> I was referring to the changelog, obviously, see how I carefully
> copy/pasted the relevant part ?

No, that's not obvious at all actually.

I would have used "Change the type to bool and remove
the now unnecessary !!" in the changelog to link Jean's
points 1 and 2, but your statement and the code and
commit log changes proposed by Jean don't match.

The type _did_ change as described in point 1.

> Stating it is a 'fix' is quite a false statement, I see no fix at all,
> maybe a cleanup, but I am not really convinced.

Perhaps "fix" is a more flexible word than you imagine.
Perhaps this a code style "fix" without logic change.

<shrug>  I'm very ambivalent about it too.

Re: [PATCH net-next] ieee802154: fix variable declaration and initializer

[Jean Sacren]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 18 Mar 2014 20:32:21 -0700
>
> On Tue, 2014-03-18 at 21:19 -0600, Jean Sacren wrote:
> 
> > 2) Fix the initializer by deleting the double logical negation
> >    operators as they don't serve any purpose.
> > 
> ...
> >  
> >  static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info)
> >  {
> > -	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
> 
> You do realize !!(a) is not equivalent to (a) ?

I admire you for your knowledge and skill. With the double logical
negation operation hereof, I do know what I'm doing.

Thank you,

-- 
Jean Sacren