Re: [PATCH 2/2] drivers: Simplify the return code

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, May 19, 2015 at 07:00:50PM +0300, Antti Palosaari wrote:
> I am also against that kind of simplifications. Even it reduces line
> or two, it makes code more inconsistent, which means you have to
> make extra thinking when reading that code. I prefer similar
> repeating patterns as much as possible.
> 
> This is how I do it usually, even there is that extra last goto.
> 
> 	ret = write_reg();
> 	if (ret)
> 		goto err;
> 
> 	ret = write_reg();
> 	if (ret)
> 		goto err;
> err:
> 	return ret;
> };
> 

I don't care too much about the original patch one way or the other.
The new code is more fashionable and fewer lines.

But these sorts of do-nothing returns are a blight.

They are misleading.  You expect goto err to do something.  You wander
what it is.  The name tells you nothing.  So you have to scroll down.
Oh crap, it's just a @#$@$@#$ waste of time do-nothing goto.  It's the
travel through a door problem, you have completely forgotten what you
are doing.

http://www.scientificamerican.com/article/why-walking-through-doorway-makes-you-forget/


And also they are a total waste of time if you care about preventing
bugs.

Some people complain about "hidden return statements" but that is only
an issue if you don't have syntax highlighting.  If you look through the
git logs it is full of places like 95f38411df055a0e ('netns: use a
spin_lock to protect nsid management') where the other coder had gotos
highlighted in the same color as regular code.  If you actually measure
how common return with lock held bugs are the goto err and the direct
return style code have equal amount of bugs.  (I have looked at this but
only briefly, so it would be interesting to see a thourough scientific
paper on it).

Also the goto err style code introduces a new class of "forgot to set
the error code" bugs which are not there in direct return code.

regards,
dan carpenter