Skip to contentSkip to conversation

staging: r8188eu: Remove unnecessary ret variable in rtw_drv_init()

PATCH In review Nathan Chancellor 4 participants 5 messages Aug 13, 2021 – Aug 16, 2021 Staging

lore
Share & tools
lore

Nathan Chancellor

author
#0 lore raw

PATCH 1/1 staging: r8188eu: Remove unnecessary ret variable in rtw_drv_init()

ret is unnecessary as both error paths set the same error code so just return that directly.

Suggested-by Fabio M. De Francesco <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>
---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/staging/r8188eu/os_dep/usb_intf.c +2 -6
index 667f41125a87..3e5f4b5eb0fc 100644
@@ -705,22 +705,18 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 {
 	struct adapter *if1 = NULL;
 	struct dvobj_priv *dvobj;
-	int ret;
 
 	/* step 0. */
 	process_spec_devid(pdid);
 
 	/* Initialize dvobj_priv */
 	dvobj = usb_dvobj_init(pusb_intf);
-	if (!dvobj) {
-		ret = -ENODEV;
+	if (!dvobj)
 		goto err;
-	}
 
 	if1 = rtw_usb_if1_init(dvobj, pusb_intf);
 	if (!if1) {
 		DBG_88E("rtw_init_primarystruct adapter Failed!\n");
-		ret = -ENODEV;
 		goto free_dvobj;
 	}
 

@@ -734,7 +730,7 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 free_dvobj:
 	usb_dvobj_deinit(pusb_intf);
 err:
-	return ret;
+	return -ENODEV;
 }
 
 /*

base-commit: 0bd35146642bdc56f1b87d75f047b1c92bd2bd39

Signature · 1 line
2.33.0.rc2
Acked-by on v1 1/1
#2 lore raw
In reply to Nathan Chancellor
Quoted 48 lines · Nathan Chancellor

ret is unnecessary as both error paths set the same error code so just return that directly.

Suggested-by Fabio M. De Francesco <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>
---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/staging/r8188eu/os_dep/usb_intf.c +2 -6
index 667f41125a87..3e5f4b5eb0fc 100644
@@ -705,22 +705,18 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 {
        struct adapter *if1 = NULL;
        struct dvobj_priv *dvobj;
-       int ret;
 
        /* step 0. */
        process_spec_devid(pdid);
 
        /* Initialize dvobj_priv */
        dvobj = usb_dvobj_init(pusb_intf);
-       if (!dvobj) {
-               ret = -ENODEV;
+       if (!dvobj)
                goto err;
-       }
 
        if1 = rtw_usb_if1_init(dvobj, pusb_intf);
        if (!if1) {
                DBG_88E("rtw_init_primarystruct adapter Failed!\n");
-               ret = -ENODEV;
                goto free_dvobj;
        }
 

@@ -734,7 +730,7 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 free_dvobj:
        usb_dvobj_deinit(pusb_intf);
 err:
-       return ret;
+       return -ENODEV;
 }
 
 /*
Quoted 4 lines

base-commit: 0bd35146642bdc56f1b87d75f047b1c92bd2bd39 --
2.33.0.rc2

Dear Nathan,

Thanks for this.

Acked-by Phillip Potter <[email protected]>

Regards,
Phil

#3 lore raw
In reply to Nathan Chancellor

To be honest, I prefered the original.

	foo = alloc();
	if (!foo) {
		ret = -EWHATEVER;
		goto free_last_thing;
	}

I like this style of error handling because all the information is there. You don't need to scroll down.

I don't really care about this specific patch at all. It's a small thing. But we had someone come through who was sort of obsessed with removing these sorts of variables. Just because you can remove a variable doesn't necessarily make the code more readable.

If you're doing the work and maintaining the driver you get to choose your own style to some extent. But I don't want to encourage people to start sending these sort of patches more generally.

regards,
dan carpenter

#4 lore raw
Quoted 11 lines · CEST Dan Carpenter

To be honest, I prefered the original.

	foo = alloc();
	if (!foo) {
		ret = -EWHATEVER;
		goto free_last_thing;
	}

I like this style of error handling because all the information is there. You don't need to scroll down.

Thinking deeper about this style of error handling, I find that Dan is quite right in preferring readability over removal of (technically unnecessary) temporary variables. Perhaps the trade-off between brevity and readability should (in general) favor the latter.

Furthermore, those temporary variables make the code easily adaptable/ extensible in cases where, in future revisions of the code, there will be more different errors to handle.

However, it's mainly a matter of style, so...

Quoted 9 lines

I don't really care about this specific patch at all. It's a small thing. But we had someone come through who was sort of obsessed with removing these sorts of variables. Just because you can remove a variable doesn't necessarily make the code more readable.

If you're doing the work and maintaining the driver you get to choose your own style to some extent.

I agree: choose your own style (to some extent).

Thanks,

Fabio

Quoted 6 lines

But I don't want to encourage people to start sending these sort of patches more generally.

regards,
dan carpenter