ieee802154: fix variable declaration and initializer

PATCH In review Jean Sacren 5 participants 9 messages Mar 19, 2014 2 messages not in the archive

Subsystems (3)

Networking Network drivers IEEE 802.15.4

lore

Share & tools

lore

Jean Sacren

author

2014-03-19 03:23 UTC #0 lore raw

PATCH 1/1 ieee802154: fix variable declaration and initializer

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 <[email protected]>

Cc Phoebe Buckheister <[email protected]>

---
 net/ieee802154/nl-phy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/ieee802154/nl-phy.c +2 -1

index 222310a07762..643dff07803b 100644
@@ -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;

A message that is not in the archive (sent off-list or not yet received): 1395199198-14310-1-git-send-email-sakiwit-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Eric Dumazet

2014-03-19 03:32 UTC #2 lore raw

Quoted 3 lines · Jean Sacren

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

...

net/ieee802154/nl-phy.c static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info) · line 382

Quoted 4 lines

static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info) {
net/ieee802154/nl-phy.c +0 -1
-	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

Joe Perches

2014-03-19 03:42 UTC #3 lore raw

Quoted 5 lines · Eric Dumazet

Quoted 3 lines · Jean Sacren
2) Fix the initializer by deleting the double logical negation operators as they don't serve any purpose.

Quoted 7 lines

...
net/ieee802154/nl-phy.c static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info) · line 382
Quoted 4 lines
static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info) {
net/ieee802154/nl-phy.c +0 -1
-	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.

Eric Dumazet

2014-03-19 04:35 UTC #4 lore raw

Quoted 8 lines · Joe Perches

Quoted 7 lines · Eric Dumazet
Quoted 3 lines · Jean Sacren
2) Fix the initializer by deleting the double logical negation operators as they don't serve any purpose.
...
Quoted 1 line

Quoted 8 lines

Quoted 5 lines
net/ieee802154/nl-phy.c static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info) · line 382
Quoted 3 lines
static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info) {
net/ieee802154/nl-phy.c +0 -1
-	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.

Jean Sacren

author

2014-03-19 05:00 UTC #5 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 18 Mar 2014 21:35:54 -0700

Quoted 16 lines · Joe Perches

Quoted 14 lines · Eric Dumazet
Quoted 12 lines · Jean Sacren
Quoted 4 lines
2) Fix the initializer by deleting the double logical negation operators as they don't serve any purpose.
...
net/ieee802154/nl-phy.c static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info) · line 382
Quoted 4 lines
static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info) {
net/ieee802154/nl-phy.c +0 -1
-	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
You do realize !!(a) is not equivalent to (a) ?

Quoted 8 lines

Quoted 2 lines
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?

Signature · 1 line

Jean Sacren

A message that is not in the archive (sent off-list or not yet received): [email protected]

Joe Perches

2014-03-19 05:11 UTC #7 lore raw

Quoted 3 lines · Jean Sacren

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

Quoted 1 line

	bool on;

2293a2293

Quoted 1 line

	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

---

Quoted 1 line

	bool on;

2305a2301,2303

Quoted 3 lines

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

Eric Dumazet

2014-03-19 05:13 UTC #8 lore raw

In reply to Jean Sacren

Quoted 3 lines · Jean Sacren

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.

Joe Perches

2014-03-19 05:11 UTC #9 lore raw

In reply to Eric Dumazet

Quoted 15 lines · Eric Dumazet

Quoted 14 lines · Joe Perches
Quoted 12 lines · Eric Dumazet
Quoted 3 lines · Jean Sacren
2) Fix the initializer by deleting the double logical negation operators as they don't serve any purpose.
...
net/ieee802154/nl-phy.c static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info) · line 382
Quoted 4 lines
static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info) {
net/ieee802154/nl-phy.c +0 -1
-	u8 on = !!nla_get_u8(info->attrs[IEEE802154_ATTR_LBT_ENABLED]);
You do realize !!(a) is not equivalent to (a) ?

Quoted 5 lines

Quoted 2 lines
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.

Quoted 2 lines

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.

Jean Sacren

author

2014-03-19 04:35 UTC #10 lore raw

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Tue, 18 Mar 2014 20:32:21 -0700

Quoted 6 lines · Jean Sacren

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

Quoted 7 lines

...
net/ieee802154/nl-phy.c static int phy_set_txpower(struct wpan_phy *phy, struct genl_info *info) · line 382
Quoted 4 lines
static int phy_set_lbt(struct wpan_phy *phy, struct genl_info *info) {
net/ieee802154/nl-phy.c +0 -1
-	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,

Signature · 1 line

Jean Sacren