| Message ID | 1363352438-15935-10-git-send-email-eduardo.valentin@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote: > Change the way the omap_bandgap_power is written so that it has only > one exit entry (Documentation/CodingStyle). > It's only if there is an unlock or something that you should do this. Otherwise the pointless bunny hop is misleading and annoying. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Dan, On 15-03-2013 17:22, Dan Carpenter wrote: > On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote: >> Change the way the omap_bandgap_power is written so that it has only >> one exit entry (Documentation/CodingStyle). >> > > It's only if there is an unlock or something that you should do > this. Otherwise the pointless bunny hop is misleading and annoying. Well, if that is the case the Chapter 7 needs to be rewritten, don't you think? The way it is stated, it is clear that it is a design decision to use it for keeping only one exit point (quoting): "Albeit deprecated by some people, the equivalent of the goto statement is used frequently by compilers in form of the unconditional jump instruction. The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done. The rationale is: - unconditional statements are easier to understand and follow - nesting is reduced - errors by not updating individual exit points when making modifications are prevented - saves the compiler work to optimize redundant code away ;)" I believe this patch falls into at least three of the above rationale. > > regards, > dan carpenter > > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 16, 2013 at 08:39:11AM -0400, Eduardo Valentin wrote: > Hey Dan, > > On 15-03-2013 17:22, Dan Carpenter wrote: > >On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote: > >>Change the way the omap_bandgap_power is written so that it has only > >>one exit entry (Documentation/CodingStyle). > >> > > > >It's only if there is an unlock or something that you should do > >this. Otherwise the pointless bunny hop is misleading and annoying. > > Well, if that is the case the Chapter 7 needs to be rewritten, don't > you think? The way it is stated, it is clear that it is a design > decision to use it for keeping only one exit point (quoting): > > "Albeit deprecated by some people, the equivalent of the goto statement is > used frequently by compilers in form of the unconditional jump instruction. > > The goto statement comes in handy when a function exits from multiple > locations and some common work such as cleanup has to be done. > > The rationale is: > > - unconditional statements are easier to understand and follow > - nesting is reduced > - errors by not updating individual exit points when making > modifications are prevented > - saves the compiler work to optimize redundant code away ;)" > > I believe this patch falls into at least three of the above rationale. There isn't any cleanup work done. That's what I was explaining about. When I see a goto I expect cleanup work to be done, but in this case it was misleading. Where you would do the one exit point is when there is cleanup: ret = function_one(); if (ret) goto unlock; ret = function_two(); if (ret) goto unlock; That's better than: ret = function_one(); if (ret) { mutex_unlock(); return ret; } ret = function_two(); if (ret) { mutex_unlock(); return ret; } On the one hand, I think the documentation should be updated because obviously people get confused by it. But on the other hand, to me the documentation is already pretty clear. There is no style guideline that says every function should only have a single return. That would be silly. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/staging/omap-thermal/omap-bandgap.c b/drivers/staging/omap-thermal/omap-bandgap.c index a8d63a2..ec79ec1 100644 --- a/drivers/staging/omap-thermal/omap-bandgap.c +++ b/drivers/staging/omap-thermal/omap-bandgap.c @@ -91,12 +91,13 @@ static int omap_bandgap_power(struct omap_bandgap *bg_ptr, bool on) int i; if (!OMAP_BANDGAP_HAS(bg_ptr, POWER_SWITCH)) - return 0; + goto exit; for (i = 0; i < bg_ptr->conf->sensor_count; i++) /* active on 0 */ RMW_BITS(bg_ptr, i, temp_sensor_ctrl, bgap_tempsoff_mask, !on); +exit: return 0; }
Change the way the omap_bandgap_power is written so that it has only one exit entry (Documentation/CodingStyle). Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com> --- drivers/staging/omap-thermal/omap-bandgap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)