On Tue, Mar 14, 2017 at 12:23:14PM +0300, Dan Carpenter wrote:
> On Tue, Mar 14, 2017 at 09:54:06AM +1100, Tobin C. Harding wrote:
> > @@ -2196,20 +2195,21 @@ static int ks_wlan_set_pmksa(struct net_device *dev,
> > case IW_PMKSA_REMOVE:
> > if (list_empty(&priv->pmklist.head)) { /* list empty */
> > return -EINVAL;
> > - } else { /* search cache data */
>
> We no longer need curly braces around the "return -EINVAL;".
>
> > - list_for_each(ptr, &priv->pmklist.head) {
> > - pmk = list_entry(ptr, struct pmk_t, list);
> > - if (!memcmp(pmksa->bssid.sa_data, pmk->bssid, ETH_ALEN)) { /* match address! list del. */
> > - eth_zero_addr(pmk->bssid);
> > - memset(pmk->pmkid, 0, IW_PMKID_LEN);
> > - list_del_init(&pmk->list);
> > - break;
> > - }
> > - }
> > - if (ptr == &priv->pmklist.head) { /* not find address. */
> > - return 0;
> > + }
> > + /* search cache data */
> > + list_for_each(ptr, &priv->pmklist.head) {
> > + pmk = list_entry(ptr, struct pmk_t, list);
> > + if (!memcmp(pmksa->bssid.sa_data, pmk->bssid, ETH_ALEN)) { /* match address! list del. */
> > + eth_zero_addr(pmk->bssid);
> > + memset(pmk->pmkid, 0, IW_PMKID_LEN);
> > + list_del_init(&pmk->list);
> > + break;
> > }
> > }
> > + if (ptr == &priv->pmklist.head) { /* not find address. */
> > + return 0;
> > + }
>
> You can keep them here because the original had curly braces so it's
> not like you're introducing a new warning.
>
> Hopefully this example explains what "one thing per patch" means a bit
> more clearly. Removing the else means you are sort of required to
> remove the curly braces so that's part of the "one thing" but fixing all
> the curly braces issues is not required even if they are on lines that
> you're modifying.
Awesome thanks. That took me three goes to work out which
braces we could do without by just looking at the diff, that is some
good diff reading skills you all have.
On the topic of "one thing per patch", so we are aiming to do one
thing per patch so it is easier to locate bugs if they get introduced
but also, and more importantly, to make review easier. So from this
patch am I right in thinking if the one thing is going to expose an
error that the reviewer is going to comment on then best to do it at
the same time so the diff is more clean? But don't do extra things
that are going to make the reviewer have to context switch?
thanks,
Tobin.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
