Message ID | 1511857593-18574-1-git-send-email-geert+renesas@glider.be |
---|---|
State | Accepted, archived |
Headers | show |
Series | of: overlay: Remove else after goto | expand |
On Tue, Nov 28, 2017 at 09:26:33AM +0100, Geert Uytterhoeven wrote: > If an "if" branch is terminated by a "goto", there's no need to have an > "else" statement and an indented block of code. > > Remove the "else" statement to simplify the code flow for the casual > reviewer. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/of/overlay.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) Applied. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/28/17 03:26, Geert Uytterhoeven wrote: > If an "if" branch is terminated by a "goto", there's no need to have an > "else" statement and an indented block of code. > > Remove the "else" statement to simplify the code flow for the casual > reviewer. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/of/overlay.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > index 8676411bd3e6f29f..53bc9e3f0b985415 100644 > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -580,9 +580,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, > of_node_put(fragment->overlay); > ret = -EINVAL; > goto err_free_fragments; > - } else { > - cnt++; > } > + > + cnt++; > } > } > > @@ -736,14 +736,12 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) > devicetree_state_flags |= DTSF_APPLY_FAIL; > } > goto err_free_overlay_changeset; > - } else { > - ret = __of_changeset_apply_notify(&ovcs->cset); > - if (ret) > - pr_err("overlay changeset entry notify error %d\n", > - ret); > - /* fall through */ > } > > + ret = __of_changeset_apply_notify(&ovcs->cset); > + if (ret) > + pr_err("overlay changeset entry notify error %d\n", ret); > + The fall through comment was removed. It is important to document that we are intentionally continuing despite the error. > list_add_tail(&ovcs->ovcs_list, &ovcs_list); > *ovcs_id = ovcs->id; > > @@ -931,15 +929,12 @@ int of_overlay_remove(int *ovcs_id) > if (ret_apply) > devicetree_state_flags |= DTSF_REVERT_FAIL; > goto out_unlock; > - } else { > - ret = __of_changeset_revert_notify(&ovcs->cset); > - if (ret) { > - pr_err("overlay changeset entry notify error %d\n", > - ret); > - /* fall through - changeset was reverted */ > - } > } > > + ret = __of_changeset_revert_notify(&ovcs->cset); > + if (ret) > + pr_err("overlay changeset entry notify error %d\n", ret); > + And same here. > *ovcs_id = 0; > > ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 29, 2017 at 3:30 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/28/17 03:26, Geert Uytterhoeven wrote: >> If an "if" branch is terminated by a "goto", there's no need to have an >> "else" statement and an indented block of code. >> >> Remove the "else" statement to simplify the code flow for the casual >> reviewer. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> drivers/of/overlay.c | 25 ++++++++++--------------- >> 1 file changed, 10 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >> index 8676411bd3e6f29f..53bc9e3f0b985415 100644 >> --- a/drivers/of/overlay.c >> +++ b/drivers/of/overlay.c >> @@ -580,9 +580,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, >> of_node_put(fragment->overlay); >> ret = -EINVAL; >> goto err_free_fragments; >> - } else { >> - cnt++; >> } >> + >> + cnt++; >> } >> } >> >> @@ -736,14 +736,12 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) >> devicetree_state_flags |= DTSF_APPLY_FAIL; >> } >> goto err_free_overlay_changeset; >> - } else { >> - ret = __of_changeset_apply_notify(&ovcs->cset); >> - if (ret) >> - pr_err("overlay changeset entry notify error %d\n", >> - ret); >> - /* fall through */ >> } >> >> + ret = __of_changeset_apply_notify(&ovcs->cset); >> + if (ret) >> + pr_err("overlay changeset entry notify error %d\n", ret); >> + > > The fall through comment was removed. It is important to document that we > are intentionally continuing despite the error. I've fixed these up like this: diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index 53bc9e3f0b98..cb520169d679 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -741,6 +741,7 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) ret = __of_changeset_apply_notify(&ovcs->cset); if (ret) pr_err("overlay changeset entry notify error %d\n", ret); + /* notify failure is not fatal, continue */ list_add_tail(&ovcs->ovcs_list, &ovcs_list); *ovcs_id = ovcs->id; @@ -934,6 +935,7 @@ int of_overlay_remove(int *ovcs_id) ret = __of_changeset_revert_notify(&ovcs->cset); if (ret) pr_err("overlay changeset entry notify error %d\n", ret); + /* notify failure is not fatal, continue */ *ovcs_id = 0; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/30/17 10:34, Rob Herring wrote: > On Wed, Nov 29, 2017 at 3:30 AM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/28/17 03:26, Geert Uytterhoeven wrote: >>> If an "if" branch is terminated by a "goto", there's no need to have an >>> "else" statement and an indented block of code. >>> >>> Remove the "else" statement to simplify the code flow for the casual >>> reviewer. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> drivers/of/overlay.c | 25 ++++++++++--------------- >>> 1 file changed, 10 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >>> index 8676411bd3e6f29f..53bc9e3f0b985415 100644 >>> --- a/drivers/of/overlay.c >>> +++ b/drivers/of/overlay.c >>> @@ -580,9 +580,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, >>> of_node_put(fragment->overlay); >>> ret = -EINVAL; >>> goto err_free_fragments; >>> - } else { >>> - cnt++; >>> } >>> + >>> + cnt++; >>> } >>> } >>> >>> @@ -736,14 +736,12 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) >>> devicetree_state_flags |= DTSF_APPLY_FAIL; >>> } >>> goto err_free_overlay_changeset; >>> - } else { >>> - ret = __of_changeset_apply_notify(&ovcs->cset); >>> - if (ret) >>> - pr_err("overlay changeset entry notify error %d\n", >>> - ret); >>> - /* fall through */ >>> } >>> >>> + ret = __of_changeset_apply_notify(&ovcs->cset); >>> + if (ret) >>> + pr_err("overlay changeset entry notify error %d\n", ret); >>> + >> >> The fall through comment was removed. It is important to document that we >> are intentionally continuing despite the error. > > I've fixed these up like this: Thanks, looks good. -Frank > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > index 53bc9e3f0b98..cb520169d679 100644 > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -741,6 +741,7 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) > ret = __of_changeset_apply_notify(&ovcs->cset); > if (ret) > pr_err("overlay changeset entry notify error %d\n", ret); > + /* notify failure is not fatal, continue */ > > list_add_tail(&ovcs->ovcs_list, &ovcs_list); > *ovcs_id = ovcs->id; > @@ -934,6 +935,7 @@ int of_overlay_remove(int *ovcs_id) > ret = __of_changeset_revert_notify(&ovcs->cset); > if (ret) > pr_err("overlay changeset entry notify error %d\n", ret); > + /* notify failure is not fatal, continue */ > > *ovcs_id = 0; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/of/overlay.c b/drivers/of/overlay.c index 8676411bd3e6f29f..53bc9e3f0b985415 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -580,9 +580,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, of_node_put(fragment->overlay); ret = -EINVAL; goto err_free_fragments; - } else { - cnt++; } + + cnt++; } } @@ -736,14 +736,12 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) devicetree_state_flags |= DTSF_APPLY_FAIL; } goto err_free_overlay_changeset; - } else { - ret = __of_changeset_apply_notify(&ovcs->cset); - if (ret) - pr_err("overlay changeset entry notify error %d\n", - ret); - /* fall through */ } + ret = __of_changeset_apply_notify(&ovcs->cset); + if (ret) + pr_err("overlay changeset entry notify error %d\n", ret); + list_add_tail(&ovcs->ovcs_list, &ovcs_list); *ovcs_id = ovcs->id; @@ -931,15 +929,12 @@ int of_overlay_remove(int *ovcs_id) if (ret_apply) devicetree_state_flags |= DTSF_REVERT_FAIL; goto out_unlock; - } else { - ret = __of_changeset_revert_notify(&ovcs->cset); - if (ret) { - pr_err("overlay changeset entry notify error %d\n", - ret); - /* fall through - changeset was reverted */ - } } + ret = __of_changeset_revert_notify(&ovcs->cset); + if (ret) + pr_err("overlay changeset entry notify error %d\n", ret); + *ovcs_id = 0; ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
If an "if" branch is terminated by a "goto", there's no need to have an "else" statement and an indented block of code. Remove the "else" statement to simplify the code flow for the casual reviewer. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/of/overlay.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)