Message ID | 1473058466-10831-1-git-send-email-hs@denx.de |
---|---|
State | Rejected |
Headers | show |
Hi Heiko, Richard, 2016-09-05 15:54 GMT+09:00 Heiko Schocher <hs@denx.de>: > From: Masahiro Yamada <yamada.masahiro@socionext.com> > > Cleanup the following code construct: > ret = expression; > if (ret) > return ret; > return 0; > > into a simple form: > return expression; > > From: Masahiro Yamada <yamada.masahiro@socionext.com> > posted on the U-Boot mailinglist. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Signed-off-by: Heiko Schocher <hs@denx.de> I am the author of the original patch in the U-Boot ML. Please notice it has not passed the review in U-Boot ML yet. Actually, I got some feedback against this patch. See http://patchwork.ozlabs.org/patch/665199/ Stephan Warren suggested that we should not break code uniformity. After I considered it and took a closer look, I decided that we should not do these changes. This patch breaks the code uniformity. See blow: > /** > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c > index 821b348..88cd61d 100644 > --- a/fs/ubifs/gc.c > +++ b/fs/ubifs/gc.c > @@ -297,10 +297,8 @@ static int sort_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb, > err = dbg_check_data_nodes_order(c, &sleb->nodes); > if (err) > return err; > - err = dbg_check_nondata_nodes_order(c, nondata); > - if (err) > - return err; > - return 0; > + > + return dbg_check_nondata_nodes_order(c, nondata); > } Original code has uniformity here. err = dbg_check_data_nodes_order(c, &sleb->nodes); if (err) return err; err = dbg_check_nondata_nodes_order(c, nondata); if (err) return err; > /** > diff --git a/fs/ubifs/lpt_commit.c b/fs/ubifs/lpt_commit.c > index ce89bdc..79a8e96 100644 > --- a/fs/ubifs/lpt_commit.c > +++ b/fs/ubifs/lpt_commit.c > @@ -313,10 +313,7 @@ static int layout_cnodes(struct ubifs_info *c) > alen = ALIGN(offs, c->min_io_size); > upd_ltab(c, lnum, c->leb_size - alen, alen - offs); > dbg_chk_lpt_sz(c, 4, alen - offs); > - err = dbg_chk_lpt_sz(c, 3, alen); > - if (err) > - return err; > - return 0; > + return dbg_chk_lpt_sz(c, 3, alen); > We have dbg_chk_lpt_sz() call just above (its return value is ignored) So, returning the value of the last dbg_chk_lpt_sz() call seems a bit weird. So, I do not want to touch this. Heiko, If you want to post this patch, it is up to you. But, in that case, could you drop my Author and Signed-off-by, then send it as your patch, please? I do not feel comfortable with my authorship for what I decided to not do. I will retract my original patch from the U-Boot ML, too.
Hello Masahiro, Am 05.09.2016 um 14:44 schrieb Masahiro Yamada: > Hi Heiko, Richard, > > > > > 2016-09-05 15:54 GMT+09:00 Heiko Schocher <hs@denx.de>: >> From: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> Cleanup the following code construct: >> ret = expression; >> if (ret) >> return ret; >> return 0; >> >> into a simple form: >> return expression; >> >> From: Masahiro Yamada <yamada.masahiro@socionext.com> >> posted on the U-Boot mailinglist. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> Signed-off-by: Heiko Schocher <hs@denx.de> > > > > I am the author of the original patch in the U-Boot ML. > > Please notice it has not passed the review in U-Boot ML yet. > Actually, I got some feedback against this patch. > > See > http://patchwork.ozlabs.org/patch/665199/ > > Stephan Warren suggested that > we should not break code uniformity. > > > After I considered it and took a closer look, > I decided that we should not do these changes. > > > This patch breaks the code uniformity. > See blow: > > > > >> /** >> diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c >> index 821b348..88cd61d 100644 >> --- a/fs/ubifs/gc.c >> +++ b/fs/ubifs/gc.c >> @@ -297,10 +297,8 @@ static int sort_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb, >> err = dbg_check_data_nodes_order(c, &sleb->nodes); >> if (err) >> return err; >> - err = dbg_check_nondata_nodes_order(c, nondata); >> - if (err) >> - return err; >> - return 0; >> + >> + return dbg_check_nondata_nodes_order(c, nondata); >> } > > Original code has uniformity here. > > > err = dbg_check_data_nodes_order(c, &sleb->nodes); > if (err) > return err; > err = dbg_check_nondata_nodes_order(c, nondata); > if (err) > return err; > > > >> /** >> diff --git a/fs/ubifs/lpt_commit.c b/fs/ubifs/lpt_commit.c >> index ce89bdc..79a8e96 100644 >> --- a/fs/ubifs/lpt_commit.c >> +++ b/fs/ubifs/lpt_commit.c >> @@ -313,10 +313,7 @@ static int layout_cnodes(struct ubifs_info *c) >> alen = ALIGN(offs, c->min_io_size); >> upd_ltab(c, lnum, c->leb_size - alen, alen - offs); >> dbg_chk_lpt_sz(c, 4, alen - offs); >> - err = dbg_chk_lpt_sz(c, 3, alen); >> - if (err) >> - return err; >> - return 0; >> + return dbg_chk_lpt_sz(c, 3, alen); >> > > We have dbg_chk_lpt_sz() call just above (its return value is ignored) > > So, returning the value of the last dbg_chk_lpt_sz() call > seems a bit weird. So, I do not want to touch this. > > > > > Heiko, > If you want to post this patch, it is up to you. > But, in that case, could you drop my Author and Signed-off-by, > then send it as your patch, please? > > I do not feel comfortable with my authorship > for what I decided to not do. > > > I will retract my original patch from the U-Boot ML, too. Oh, then I was a little to fast ... sorry. @Richard: Should we just forget this patch? bye, Heiko
On 09/05/2016 08:54 AM, Heiko Schocher wrote: > diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c > index 11a11b3..48d6851 100644 > --- a/fs/ubifs/budget.c > +++ b/fs/ubifs/budget.c > @@ -77,7 +77,7 @@ static void shrink_liability(struct ubifs_info *c, int nr_to_write) > */ > static int run_gc(struct ubifs_info *c) > { > - int err, lnum; > + int lnum; > > /* Make some free space by garbage-collecting dirty space */ > down_read(&c->commit_sem); > @@ -88,10 +88,7 @@ static int run_gc(struct ubifs_info *c) > > /* GC freed one LEB, return it to lprops */ > dbg_budg("GC freed LEB %d", lnum); > - err = ubifs_return_leb(c, lnum); > - if (err) > - return err; > - return 0; > + return = ubifs_return_leb(c, lnum); > } > Apart from the other issues discussed below and in v1, I don't believe that this _ever_ compiled successfully.
2016-09-05 22:18 GMT+09:00 David Oberhollenzer <david.oberhollenzer@sigma-star.at>: > On 09/05/2016 08:54 AM, Heiko Schocher wrote: >> diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c >> index 11a11b3..48d6851 100644 >> --- a/fs/ubifs/budget.c >> +++ b/fs/ubifs/budget.c >> @@ -77,7 +77,7 @@ static void shrink_liability(struct ubifs_info *c, int nr_to_write) >> */ >> static int run_gc(struct ubifs_info *c) >> { >> - int err, lnum; >> + int lnum; >> >> /* Make some free space by garbage-collecting dirty space */ >> down_read(&c->commit_sem); >> @@ -88,10 +88,7 @@ static int run_gc(struct ubifs_info *c) >> >> /* GC freed one LEB, return it to lprops */ >> dbg_budg("GC freed LEB %d", lnum); >> - err = ubifs_return_leb(c, lnum); >> - if (err) >> - return err; >> - return 0; >> + return = ubifs_return_leb(c, lnum); >> } >> > > Apart from the other issues discussed below and in v1, I don't > believe that this _ever_ compiled successfully. > Just in case: It was not me who added '=' after 'return'. (I usually run build-test before sending my patches.)
On 05.09.2016 15:05, Heiko Schocher wrote:
> @Richard: Should we just forget this patch?
Let's drop it for now.
It caused already a way more churn than a trivial style cleanup
patch should...
Thanks,
//richard
Hello Richard, Am 05.09.2016 um 15:32 schrieb Richard Weinberger: > On 05.09.2016 15:05, Heiko Schocher wrote: >> @Richard: Should we just forget this patch? > > Let's drop it for now. > It caused already a way more churn than a trivial style cleanup > patch should... Yes! It was a too fast shoot ... sorry for the noise! bye, Heiko
Hi Masahiro, [auto build test ERROR on linus/master] [also build test ERROR on v4.8-rc5 next-20160909] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Heiko-Schocher/ubifs-compress-lines-for-immediate-return/20160905-145802 config: i386-randconfig-x0-09110426 (attached as .config) compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): fs/ubifs/budget.c: In function 'run_gc': >> fs/ubifs/budget.c:91:9: error: expected expression before '=' token return = ubifs_return_leb(c, lnum); ^ fs/ubifs/budget.c:92:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ vim +91 fs/ubifs/budget.c 85 up_read(&c->commit_sem); 86 if (lnum < 0) 87 return lnum; 88 89 /* GC freed one LEB, return it to lprops */ 90 dbg_budg("GC freed LEB %d", lnum); > 91 return = ubifs_return_leb(c, lnum); 92 } 93 94 /** --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c index 11a11b3..48d6851 100644 --- a/fs/ubifs/budget.c +++ b/fs/ubifs/budget.c @@ -77,7 +77,7 @@ static void shrink_liability(struct ubifs_info *c, int nr_to_write) */ static int run_gc(struct ubifs_info *c) { - int err, lnum; + int lnum; /* Make some free space by garbage-collecting dirty space */ down_read(&c->commit_sem); @@ -88,10 +88,7 @@ static int run_gc(struct ubifs_info *c) /* GC freed one LEB, return it to lprops */ dbg_budg("GC freed LEB %d", lnum); - err = ubifs_return_leb(c, lnum); - if (err) - return err; - return 0; + return = ubifs_return_leb(c, lnum); } /** diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c index 821b348..88cd61d 100644 --- a/fs/ubifs/gc.c +++ b/fs/ubifs/gc.c @@ -297,10 +297,8 @@ static int sort_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb, err = dbg_check_data_nodes_order(c, &sleb->nodes); if (err) return err; - err = dbg_check_nondata_nodes_order(c, nondata); - if (err) - return err; - return 0; + + return dbg_check_nondata_nodes_order(c, nondata); } /** diff --git a/fs/ubifs/lpt_commit.c b/fs/ubifs/lpt_commit.c index ce89bdc..79a8e96 100644 --- a/fs/ubifs/lpt_commit.c +++ b/fs/ubifs/lpt_commit.c @@ -313,10 +313,7 @@ static int layout_cnodes(struct ubifs_info *c) alen = ALIGN(offs, c->min_io_size); upd_ltab(c, lnum, c->leb_size - alen, alen - offs); dbg_chk_lpt_sz(c, 4, alen - offs); - err = dbg_chk_lpt_sz(c, 3, alen); - if (err) - return err; - return 0; + return dbg_chk_lpt_sz(c, 3, alen); no_space: ubifs_err(c, "LPT out of space at LEB %d:%d needing %d, done_ltab %d, done_lsave %d",