| Message ID | 1395199198-14310-1-git-send-email-sakiwit@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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) ? -- 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
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. -- 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
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,
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. -- 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
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?
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. -- 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
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 < --- -- 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
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. -- 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
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;
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(-) -- 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