Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1476152 > unrolled thread

[PATCH v2] ubifs: compress lines for immediate return

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] ubifs: compress lines for immediate return Heiko Schocher <hs@denx.de> - 2016-09-05 09:00 +0200
    Re: [PATCH v2] ubifs: compress lines for immediate return Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-09-05 14:50 +0200
      Re: [PATCH v2] ubifs: compress lines for immediate return Heiko Schocher <hs@denx.de> - 2016-09-05 15:10 +0200
        Re: [PATCH v2] ubifs: compress lines for immediate return Richard Weinberger <richard@nod.at> - 2016-09-05 15:40 +0200
          Re: [PATCH v2] ubifs: compress lines for immediate return Heiko Schocher <hs@denx.de> - 2016-09-06 06:40 +0200
    Re: [PATCH v2] ubifs: compress lines for immediate return David Oberhollenzer <david.oberhollenzer@sigma-star.at> - 2016-09-05 15:20 +0200
      Re: [PATCH v2] ubifs: compress lines for immediate return Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-09-05 15:30 +0200

#1476152 — [PATCH v2] ubifs: compress lines for immediate return

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>
---

Changes in v2:
- add comment from Richard Weinberger:
  rephrase commit message
  add Masahiros "Signed-off-by" tag.

 fs/ubifs/budget.c     | 7 ++-----
 fs/ubifs/gc.c         | 6 ++----
 fs/ubifs/lpt_commit.c | 5 +----
 3 files changed, 5 insertions(+), 13 deletions(-)

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",
-- 
2.5.5

[toc] | [next] | [standalone]

#1476386

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.




-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1476397

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[toc] | [prev] | [next] | [standalone]

#1476461

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

[toc] | [prev] | [next] | [standalone]

#1477064

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[toc] | [prev] | [next] | [standalone]

#1476404

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.

[toc] | [prev] | [next] | [standalone]

#1476449

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.)





-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web