Hi Dan,
On 27/01/20 1:07 pm, Dan Carpenter wrote:
>
> Looks good.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> On Thu, Jan 23, 2020 at 12:50:47PM +0000, Ajay.Kathat@xxxxxxxxxxxxx wrote:
>> @@ -384,19 +378,18 @@ static int wilc_sdio_write_reg(struct wilc *wilc, u32 addr, u32 data)
>> cmd.address = addr;
>> cmd.data = data;
>> ret = wilc_sdio_cmd52(wilc, &cmd);
>> - if (ret) {
>> + if (ret)
>> dev_err(&func->dev,
>> "Failed cmd 52, read reg (%08x) ...\n", addr);
>> - goto fail;
>> - }
>
> Please don't resend, but try to avoid this sort of anti-pattern in the
> future. You're treating the last failure condition in the function as
> special. In this case it's particularly difficult to read because we
> are far from the bottom of the function, but even if we were right at
> the bottom, I would try to avoid it.
>
> I am extreme enough that I would avoid even things like:
>
> ret = frob();
> if (ret)
> printf("ret failed\n");
> return ret;
>
> Instead I would write:
>
> ret = frob();
> if (ret) {
> printf("ret failed\n");
> return ret;
> }
> return 0;
>
> It's just nice to have the error path at indent level 2 and the
> success path at indent level 1. And the other thing that I like is the
> BIG BOLD "return 0;" at the end of the function. Some functions return
> positive numbers on success so if I see "return result;" then I have to
> look back a few lines to see if "result" can be positive.
>
> The other anti-pattern which people often do is success handling
> (instead of error handling) for the last error condition in a function.
>
> ret = one();
> if (ret)
> return ret;
> ret = two();
> if (ret)
> return ret;
> ret = three();
> if (!ret)
> ret = four();
> return ret;
>
Thanks for your useful advice to avoid anti-pattern. I shall keep these
points in mind while working on future patches.
Regards,
Ajay
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
