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

Groups > linux.kernel > #1185750 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-07-16 13:30 +0200
    Re: [PATCH 3/6] staging: rtl8188eu: remove goto label Dan Carpenter <dan.carpenter@oracle.com> - 2015-07-17 13:10 +0200
      Re: [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-07-17 13:30 +0200
        Re: [PATCH 3/6] staging: rtl8188eu: remove goto label Dan Carpenter <dan.carpenter@oracle.com> - 2015-07-17 13:50 +0200
          Re: [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-07-17 14:10 +0200

#1185750 — [PATCH 3/6] staging: rtl8188eu: remove goto label

By checking for the success of kzalloc we were able to remove the goto
label thus making the code more readable.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 21744a5..f68875b 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -115,14 +115,11 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
 	mutex_init(&pdvobjpriv->usb_vendor_req_mutex);
 	pdvobjpriv->usb_vendor_req_buf = kzalloc(MAX_USB_IO_CTL_SIZE, GFP_KERNEL);
 
-	if (!pdvobjpriv->usb_vendor_req_buf)
-		goto free_dvobj;
-
-	usb_get_dev(pusbd);
-
-	status = _SUCCESS;
+	if (pdvobjpriv->usb_vendor_req_buf) {
+		usb_get_dev(pusbd);
+		status = _SUCCESS;
+	}
 
-free_dvobj:
 	if (status != _SUCCESS) {
 		usb_set_intfdata(usb_intf, NULL);
 		kfree(pdvobjpriv);
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1186712

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

It should look like:

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index d0d4335..7617a22 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -120,16 +120,13 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
 
 	usb_get_dev(pusbd);
 
-	status = _SUCCESS;
+	return pdvobjpriv;
 
 free_dvobj:
-	if (status != _SUCCESS && pdvobjpriv) {
-		usb_set_intfdata(usb_intf, NULL);
-		kfree(pdvobjpriv);
-		pdvobjpriv = NULL;
-	}
-exit:
-	return pdvobjpriv;
+	usb_set_intfdata(usb_intf, NULL);
+	kfree(pdvobjpriv);
+
+	return NULL;
 }
 
 static void usb_dvobj_deinit(struct usb_interface *usb_intf)


See?  No indenting, no if statements.  Just one statement after another.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1186724

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?

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1186733

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1186752

On Fri, Jul 17, 2015 at 02:47:55PM +0300, Dan Carpenter wrote:
> 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.
understood.
I was planning another series for that file so i will include one for
this also. But before that I am going to send in the patch for sm7xxfb
to move out of staging. Please check that.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web