[U-Boot] [PATCH] treewide: compress lines for immediate return

Masahiro Yamada yamada.masahiro at socionext.com
Wed Sep 7 03:14:28 CEST 2016


Hi Stephen,

2016-09-07 1:09 GMT+09:00 Stephen Warren <swarren at wwwdotorg.org>:

>>> All child function calls are structured the same, so someone reading the
>>> code will always see the same structure irrespective of where in a
>>> function
>>> a child function is called. This gives uniformity. This also yields a few
>>> maintenance advantages below, and helps keep all code uniform even if any
>>> of
>>> the maintenance operations below have been applied to some functions and
>>> aren't needed in others.
>>
>>
>>
>> Did you think I ran a semantic patch with Coccinelle
>> and then sent it blindly?
>>
>> No, this patch passed my eyes' check, at least.
>>
>> Please notice this patch did not transform
>> the following function in arch/arm/cpu/armv7/am33xx/clk_synthesizer.c
>
> ...
>
> The patch description clearly stated that the patch was purely the result of
> applying the Coccinelle script. If there were exceptions or other edits,
> they should have been explicitly mentioned too.

Right.
The git-log implied a semantic patch, but I did not mention that
I sent the output of Coccinelle as is.

Actually, I cherry-picked reasonable hunks.

Coccinelle may sometimes do false positive jobs (or undesirable output
like this case),
so I think "Coccinelle + checking by eyes" is a good practice.


I dropped a semantic patch snippet from v3 git-log.



>> If we start to consider things that may happen or may not happen,
>> we end up with adding redundancy all the time.
>>
>> Are you positive or negative for the following hunk?
>>
>>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>> b/drivers/mtd/nand/fsl_elbc_nand.c
>>> index f621f14..b27a6af 100644
>>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>> @@ -788,11 +788,7 @@ static int fsl_elbc_chip_init(int devnum, u8 *addr)
>>>         if (ret)
>>>                 return ret;
>>>
>>> -       ret = nand_register(devnum, mtd);
>>> -       if (ret)
>>> -               return ret;
>>> -
>>> -       return 0;
>>> +       return nand_register(devnum, mtd);
>>>  }
>
>
> I'd probably tend not to do that particular conversion, for consistency with
> the immediately preceding nand_scan_tail() case. Still, this one isn't such
> an obvious call so I wouldn't feel particularly strongly about it,
> especially as it isn't a driver I work on.
>
>> I think probe/init function can return a value
>> of register function directly, from my best common sense.
>>
>> This change will lose 2)
>> in case fsl_elbc_chip_init() fails to do nand_register, though.

OK.  I dropped those changes in v2.

(I still personally believe "return *_register();" is good coding style, though.
This might be a matter of preference...)


In v3, I only fixed drivers/video/vidconsole-uclass.c
because I thought it is simple enough.
http://patchwork.ozlabs.org/patch/666560/

and it was acked by Anatolij.




-- 
Best Regards
Masahiro Yamada


More information about the U-Boot mailing list