| Message ID | 1400473875-22228-5-git-send-email-Julia.Lawall@lip6.fr |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Am 19.05.2014 06:31, schrieb Julia Lawall: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > Delete unnecessary local variable whose value is always 0 and that hides > the fact that the result is always 0. > > A simplified version of the semantic patch that fixes this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @r exists@ > local idexpression ret; > expression e; > position p; > @@ > > -ret = 0; > ... when != ret = e > return > - ret > + 0 > ; > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > Alternatively, is an error code wanted under the if? > > drivers/net/wimax/i2400m/driver.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c > index 9c34d2f..901a534 100644 > --- a/drivers/net/wimax/i2400m/driver.c > +++ b/drivers/net/wimax/i2400m/driver.c > @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier, > */ > int i2400m_pre_reset(struct i2400m *i2400m) > { > - int result; > struct device *dev = i2400m_dev(i2400m); > > d_fnstart(3, dev, "(i2400m %p)\n", i2400m); > d_printf(1, dev, "pre-reset shut down\n"); > > - result = 0; > mutex_lock(&i2400m->init_mutex); > if (i2400m->updown) { > netif_tx_disable(i2400m->wimax_dev.net_dev); > __i2400m_dev_stop(i2400m); > - result = 0; > /* down't set updown to zero -- this way > * post_reset can restore properly */ > } > mutex_unlock(&i2400m->init_mutex); > if (i2400m->bus_release) > i2400m->bus_release(i2400m); > - d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result); > - return result; > + d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0); d_fnend(3, dev, "i2400m=%p\n", i2400m); or something like that re, wh > + return 0; > } > EXPORT_SYMBOL_GPL(i2400m_pre_reset); > > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 Mon, 19 May 2014, walter harms wrote: > > > Am 19.05.2014 06:31, schrieb Julia Lawall: > > From: Julia Lawall <Julia.Lawall@lip6.fr> > > > > Delete unnecessary local variable whose value is always 0 and that hides > > the fact that the result is always 0. > > > > A simplified version of the semantic patch that fixes this problem is as > > follows: (http://coccinelle.lip6.fr/) > > > > // <smpl> > > @r exists@ > > local idexpression ret; > > expression e; > > position p; > > @@ > > > > -ret = 0; > > ... when != ret = e > > return > > - ret > > + 0 > > ; > > // </smpl> > > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > > > --- > > Alternatively, is an error code wanted under the if? > > > > drivers/net/wimax/i2400m/driver.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c > > index 9c34d2f..901a534 100644 > > --- a/drivers/net/wimax/i2400m/driver.c > > +++ b/drivers/net/wimax/i2400m/driver.c > > @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier, > > */ > > int i2400m_pre_reset(struct i2400m *i2400m) > > { > > - int result; > > struct device *dev = i2400m_dev(i2400m); > > > > d_fnstart(3, dev, "(i2400m %p)\n", i2400m); > > d_printf(1, dev, "pre-reset shut down\n"); > > > > - result = 0; > > mutex_lock(&i2400m->init_mutex); > > if (i2400m->updown) { > > netif_tx_disable(i2400m->wimax_dev.net_dev); > > __i2400m_dev_stop(i2400m); > > - result = 0; > > /* down't set updown to zero -- this way > > * post_reset can restore properly */ > > } > > mutex_unlock(&i2400m->init_mutex); > > if (i2400m->bus_release) > > i2400m->bus_release(i2400m); > > - d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result); > > - return result; > > + d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0); > > > d_fnend(3, dev, "i2400m=%p\n", i2400m); All the other logging messages in this file seem to have the form (i2400m %p) = %d or (i2400m %p) = void in the case of a function that has no return value. I don't know if a return value is wanted here, but since the function is exported, perhaps it is best not to change its type. julia > or something like that > re, > wh > > > + return 0; > > } > > EXPORT_SYMBOL_GPL(i2400m_pre_reset); > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- 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 19-05-2014 8:31, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > Delete unnecessary local variable whose value is always 0 and that hides > the fact that the result is always 0. > A simplified version of the semantic patch that fixes this problem is as > follows: (http://coccinelle.lip6.fr/) > // <smpl> > @r exists@ > local idexpression ret; > expression e; > position p; > @@ > > -ret = 0; > ... when != ret = e > return > - ret > + 0 > ; > // </smpl> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > --- > Alternatively, is an error code wanted under the if? > drivers/net/wimax/i2400m/driver.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c > index 9c34d2f..901a534 100644 > --- a/drivers/net/wimax/i2400m/driver.c > +++ b/drivers/net/wimax/i2400m/driver.c > @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier, > */ > int i2400m_pre_reset(struct i2400m *i2400m) > { > - int result; > struct device *dev = i2400m_dev(i2400m); > > d_fnstart(3, dev, "(i2400m %p)\n", i2400m); > d_printf(1, dev, "pre-reset shut down\n"); > > - result = 0; > mutex_lock(&i2400m->init_mutex); > if (i2400m->updown) { > netif_tx_disable(i2400m->wimax_dev.net_dev); > __i2400m_dev_stop(i2400m); > - result = 0; > /* down't set updown to zero -- this way > * post_reset can restore properly */ > } > mutex_unlock(&i2400m->init_mutex); > if (i2400m->bus_release) > i2400m->bus_release(i2400m); > - d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result); > - return result; > + d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0); Again, why not just "(i2400m %p) = 0\n"? > + return 0; WBR, Sergei -- 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
Am 19.05.2014 13:30, schrieb Julia Lawall: > > > On Mon, 19 May 2014, walter harms wrote: > >> >> >> Am 19.05.2014 06:31, schrieb Julia Lawall: >>> From: Julia Lawall <Julia.Lawall@lip6.fr> >>> >>> Delete unnecessary local variable whose value is always 0 and that hides >>> the fact that the result is always 0. >>> >>> A simplified version of the semantic patch that fixes this problem is as >>> follows: (http://coccinelle.lip6.fr/) >>> >>> // <smpl> >>> @r exists@ >>> local idexpression ret; >>> expression e; >>> position p; >>> @@ >>> >>> -ret = 0; >>> ... when != ret = e >>> return >>> - ret >>> + 0 >>> ; >>> // </smpl> >>> >>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> >>> >>> --- >>> Alternatively, is an error code wanted under the if? >>> >>> drivers/net/wimax/i2400m/driver.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c >>> index 9c34d2f..901a534 100644 >>> --- a/drivers/net/wimax/i2400m/driver.c >>> +++ b/drivers/net/wimax/i2400m/driver.c >>> @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier, >>> */ >>> int i2400m_pre_reset(struct i2400m *i2400m) >>> { >>> - int result; >>> struct device *dev = i2400m_dev(i2400m); >>> >>> d_fnstart(3, dev, "(i2400m %p)\n", i2400m); >>> d_printf(1, dev, "pre-reset shut down\n"); >>> >>> - result = 0; >>> mutex_lock(&i2400m->init_mutex); >>> if (i2400m->updown) { >>> netif_tx_disable(i2400m->wimax_dev.net_dev); >>> __i2400m_dev_stop(i2400m); >>> - result = 0; >>> /* down't set updown to zero -- this way >>> * post_reset can restore properly */ >>> } >>> mutex_unlock(&i2400m->init_mutex); >>> if (i2400m->bus_release) >>> i2400m->bus_release(i2400m); >>> - d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result); >>> - return result; >>> + d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0); >> >> >> d_fnend(3, dev, "i2400m=%p\n", i2400m); > > All the other logging messages in this file seem to have the form > (i2400m %p) = %d > or (i2400m %p) = void in the case of a function that has no return value. > I don't know if a return value is wanted here, but since the function is > exported, perhaps it is best not to change its type. I got my "inspiration" from d_fnstart(). As i see it the whole function is void so printing a debug msg with a bogus return value may confuse some people. re, wh > julia > >> or something like that >> re, >> wh >> >>> + return 0; >>> } >>> EXPORT_SYMBOL_GPL(i2400m_pre_reset); >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > -- 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/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c index 9c34d2f..901a534 100644 --- a/drivers/net/wimax/i2400m/driver.c +++ b/drivers/net/wimax/i2400m/driver.c @@ -500,26 +500,23 @@ int i2400m_pm_notifier(struct notifier_block *notifier, */ int i2400m_pre_reset(struct i2400m *i2400m) { - int result; struct device *dev = i2400m_dev(i2400m); d_fnstart(3, dev, "(i2400m %p)\n", i2400m); d_printf(1, dev, "pre-reset shut down\n"); - result = 0; mutex_lock(&i2400m->init_mutex); if (i2400m->updown) { netif_tx_disable(i2400m->wimax_dev.net_dev); __i2400m_dev_stop(i2400m); - result = 0; /* down't set updown to zero -- this way * post_reset can restore properly */ } mutex_unlock(&i2400m->init_mutex); if (i2400m->bus_release) i2400m->bus_release(i2400m); - d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, result); - return result; + d_fnend(3, dev, "(i2400m %p) = %d\n", i2400m, 0); + return 0; } EXPORT_SYMBOL_GPL(i2400m_pre_reset);