Re: [PATCH 3/6] staging: rtl8188eu: remove goto label

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

On Fri, Jul 17, 2015 at 04:55:12PM +0530, Sudip Mukherjee wrote:
> On Fri, Jul 17, 2015 at 02:03:48PM +0300, Dan Carpenter wrote:
> > On Thu, Jul 16, 2015 at 04:58:09PM +0530, Sudip Mukherjee wrote:
> > > By checking for the success of kzalloc we were able to remove the goto
> > > label thus making the code more readable.
> > > 
> > 
> > No...  You've just changed error handling to success handling and added
> > some new indent levels and made a tangled spaghetti exit path even more
> > tangled.  Spoderman wants to know, "Why u do dis?"
> I had to go for google to lookup Spoderman or Spiderman. :)
> At the end of the series, isn't the code better looking and simpler
> than the original code?

Yes, but that's not a very high bar to clear.

What I'm trying to explain is the beauty of writing one statement after
the other without indenting.  The simplest kind of code looks like this
without error handling.


int some_function(void)
{
	do_thing_one();
	do_thing_two();
	do_thing_three();

	return 0;
}

When you add canonical error handling it looks like this:

int some_function(void)
{
	do_thing_one();
	if (failure)
		return -ERROR;

	do_thing_two();
	if (failure)
		goto undo_one;

	do_thing_three();
	if (failure)
		goto undo_two;

	return 0;

undo_two:
	undo_two_thing();
undo_one:
	undo_one_thing();

	return -ERROR;
}

But with success handling it look like this:

int some_function(void)
{
	do_thing_one();
	if (success) {
		do_thing_two();
		if (success) {
			do_thing_three();
			if (success)
				return 0;
		}
	}
	return -ERROR;
}

Fewer lines, but terrible code.  In this patch we only make the last
condition success handling but it's still messier than need be.

	if (fail)
	if (fail)
	if (fail)
	if (success)
	else

The canonical method is more consistent and simplest to read.

regards,
dan carpenter