[PATCH] Removed useless retval variables in usb-serial.c

[PATCH] Removed useless retval variables in usb-serial.c

[Trevor Pace]

Removed useless return value variables.

Signed-off By: Trevor Pace <trevor.pace@dal.ca>

================================================================================

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index bd7581b..faec1d1 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -362,10 +362,9 @@ static int serial_write(struct tty_struct *tty, const
unsigned char *buf,
 								int count)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	int retval = -ENODEV;

 	if (port->serial->dev->state == USB_STATE_NOTATTACHED)
-		goto exit;
+		return -ENODEV;

 	dbg("%s - port %d, %d byte(s)", __func__, port->number, count);

@@ -374,10 +373,7 @@ static int serial_write(struct tty_struct *tty, const
unsigned char *buf,
 	WARN_ON(!port->port.count);

 	/* pass on to the driver specific version of this function */
-	retval = port->serial->type->write(tty, port, buf, count);
-
-exit:
-	return retval;
+	return port->serial->type->write(tty, port, buf, count);
 }

 static int serial_write_room(struct tty_struct *tty)
@@ -429,7 +425,6 @@ static int serial_ioctl(struct tty_struct *tty, struct file
*file,
 					unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	int retval = -ENODEV;

 	dbg("%s - port %d, cmd 0x%.4x", __func__, port->number, cmd);

@@ -437,11 +432,9 @@ static int serial_ioctl(struct tty_struct *tty, struct file
*file,

 	/* pass on to the driver specific version of this function
 	   if it is available */
-	if (port->serial->type->ioctl) {
-		retval = port->serial->type->ioctl(tty, file, cmd, arg);
-	} else
-		retval = -ENOIOCTLCMD;
-	return retval;
+	if (port->serial->type->ioctl)
+		return port->serial->type->ioctl(tty, file, cmd, arg);
+        return -ENOIOCTLCMD;
 }

 static void serial_set_termios(struct tty_struct *tty, struct ktermios *old)
@@ -1174,7 +1167,7 @@ int usb_serial_suspend(struct usb_interface *intf,
pm_message_t message)
 {
 	struct usb_serial *serial = usb_get_intfdata(intf);
 	struct usb_serial_port *port;
-	int i, r = 0;
+	int i;

 	serial->suspending = 1;

@@ -1185,24 +1178,21 @@ int usb_serial_suspend(struct usb_interface *intf,
pm_message_t message)
 	}

 	if (serial->type->suspend)
-		r = serial->type->suspend(serial, message);
+		return serial->type->suspend(serial, message);

-	return r;
+	return 0;
 }
 EXPORT_SYMBOL(usb_serial_suspend);

 int usb_serial_resume(struct usb_interface *intf)
 {
 	struct usb_serial *serial = usb_get_intfdata(intf);
-	int rv;

 	serial->suspending = 0;
-	if (serial->type->resume)
-		rv = serial->type->resume(serial);
-	else
-		rv = usb_serial_generic_resume(serial);

-	return rv;
+	return (serial->type->resume)
+		? serial->type->resume(serial)
+		: usb_serial_generic_resume(serial);
 }
 EXPORT_SYMBOL(usb_serial_resume);

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Sergei Shtylyov]

Hello.

Trevor Pace wrote:

> Removed useless return value variables.

    Are you sure gcc doesn't optimize them away? :-)

> Signed-off By: Trevor Pace <trevor.pace@dal.ca>

> ================================================================================

> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> index bd7581b..faec1d1 100644
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c

[...]

> @@ -437,11 +432,9 @@ static int serial_ioctl(struct tty_struct *tty, struct file
> *file,
> 
>  	/* pass on to the driver specific version of this function
>  	   if it is available */
> -	if (port->serial->type->ioctl) {
> -		retval = port->serial->type->ioctl(tty, file, cmd, arg);
> -	} else
> -		retval = -ENOIOCTLCMD;
> -	return retval;
> +	if (port->serial->type->ioctl)
> +		return port->serial->type->ioctl(tty, file, cmd, arg);
> +        return -ENOIOCTLCMD;

    Spaces instead of tab here...

> @@ -1185,24 +1178,21 @@ int usb_serial_suspend(struct usb_interface *intf,
> pm_message_t message)
>  	}
> 
>  	if (serial->type->suspend)
> -		r = serial->type->suspend(serial, message);
> +		return serial->type->suspend(serial, message);
> 
> -	return r;
> +	return 0;
>  }
>  EXPORT_SYMBOL(usb_serial_suspend);
> 
>  int usb_serial_resume(struct usb_interface *intf)
>  {
>  	struct usb_serial *serial = usb_get_intfdata(intf);
> -	int rv;
> 
>  	serial->suspending = 0;
> -	if (serial->type->resume)
> -		rv = serial->type->resume(serial);
> -	else
> -		rv = usb_serial_generic_resume(serial);
> 
> -	return rv;
> +	return (serial->type->resume)

    Parens totally not needed here.

> +		? serial->type->resume(serial)
> +		: usb_serial_generic_resume(serial);
>  }
>  EXPORT_SYMBOL(usb_serial_resume);

WBR, Sergei

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Oliver Neukum]

Am Freitag, 24. Juli 2009 17:48:03 schrieb Trevor Pace:
> Removed useless return value variables.
>
> Signed-off By: Trevor Pace <trevor.pace@dal.ca>

These changes are a bad idea. They'll bite us in the ass as soon as we
change locking or need to do more cleanups. The preferred form of
error handling is

r = op()
if (r < 0)
	goto error_exit;

I suggest that you don't apply them.

	Regards
		Oliver

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Trevor Pace]

Hey,

My university email account apparently doesn't like maintaining my
formatting, so I'm gonna resend this from my gmail.

Parenthesis may not be needed for that return check but I find it
makes it easier to tell that you are doing. Otherwise someone might
think you are returning serial->type->resume because it has been
broken onto different lines. I checked over the
Documentation/CodingStyle file and found no conflicts with that sort
of notation.

Trevor Pace

Lets try this again:

********************************************************************

Removed useless return value variables.

Signed-off By: Trevor Pace <trevor.pace@dal.ca>

================================================================================

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index bd7581b..faec1d1 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -362,10 +362,9 @@ static int serial_write(struct tty_struct *tty,
const unsigned char *buf,
 								int count)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	int retval = -ENODEV;

 	if (port->serial->dev->state == USB_STATE_NOTATTACHED)
-		goto exit;
+		return -ENODEV;

 	dbg("%s - port %d, %d byte(s)", __func__, port->number, count);

@@ -374,10 +373,7 @@ static int serial_write(struct tty_struct *tty,
const unsigned char *buf,
 	WARN_ON(!port->port.count);

 	/* pass on to the driver specific version of this function */
-	retval = port->serial->type->write(tty, port, buf, count);
-
-exit:
-	return retval;
+	return port->serial->type->write(tty, port, buf, count);
 }

 static int serial_write_room(struct tty_struct *tty)
@@ -429,7 +425,6 @@ static int serial_ioctl(struct tty_struct *tty,
struct file *file,
 					unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	int retval = -ENODEV;

 	dbg("%s - port %d, cmd 0x%.4x", __func__, port->number, cmd);

@@ -437,11 +432,9 @@ static int serial_ioctl(struct tty_struct *tty,
struct file *file,

 	/* pass on to the driver specific version of this function
 	   if it is available */
-	if (port->serial->type->ioctl) {
-		retval = port->serial->type->ioctl(tty, file, cmd, arg);
-	} else
-		retval = -ENOIOCTLCMD;
-	return retval;
+	if (port->serial->type->ioctl)
+		return port->serial->type->ioctl(tty, file, cmd, arg);
+	return -ENOIOCTLCMD;
 }

 static void serial_set_termios(struct tty_struct *tty, struct ktermios *old)
@@ -1174,7 +1167,7 @@ int usb_serial_suspend(struct usb_interface
*intf, pm_message_t message)
 {
 	struct usb_serial *serial = usb_get_intfdata(intf);
 	struct usb_serial_port *port;
-	int i, r = 0;
+	int i;

 	serial->suspending = 1;

@@ -1185,24 +1178,21 @@ int usb_serial_suspend(struct usb_interface
*intf, pm_message_t message)
 	}

 	if (serial->type->suspend)
-		r = serial->type->suspend(serial, message);
+		return serial->type->suspend(serial, message);

-	return r;
+	return 0;
 }
 EXPORT_SYMBOL(usb_serial_suspend);

 int usb_serial_resume(struct usb_interface *intf)
 {
 	struct usb_serial *serial = usb_get_intfdata(intf);
-	int rv;

 	serial->suspending = 0;
-	if (serial->type->resume)
-		rv = serial->type->resume(serial);
-	else
-		rv = usb_serial_generic_resume(serial);

-	return rv;
+	return (serial->type->resume)
+		? serial->type->resume(serial)
+		: usb_serial_generic_resume(serial);
 }
 EXPORT_SYMBOL(usb_serial_resume);


******************************************************************


On Fri, Jul 24, 2009 at 1:29 PM, Sergei Shtylyov<sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Trevor Pace wrote:
>
>> Removed useless return value variables.
>
>   Are you sure gcc doesn't optimize them away? :-)
>
>> Signed-off By: Trevor Pace <trevor.pace@dal.ca>
>
>>
>> ================================================================================
>
>> diff --git a/drivers/usb/serial/usb-serial.c
>> b/drivers/usb/serial/usb-serial.c
>> index bd7581b..faec1d1 100644
>> --- a/drivers/usb/serial/usb-serial.c
>> +++ b/drivers/usb/serial/usb-serial.c
>
> [...]
>
>> @@ -437,11 +432,9 @@ static int serial_ioctl(struct tty_struct *tty,
>> struct file
>> *file,
>>
>>        /* pass on to the driver specific version of this function
>>           if it is available */
>> -       if (port->serial->type->ioctl) {
>> -               retval = port->serial->type->ioctl(tty, file, cmd, arg);
>> -       } else
>> -               retval = -ENOIOCTLCMD;
>> -       return retval;
>> +       if (port->serial->type->ioctl)
>> +               return port->serial->type->ioctl(tty, file, cmd, arg);
>> +        return -ENOIOCTLCMD;
>
>   Spaces instead of tab here...
>
>> @@ -1185,24 +1178,21 @@ int usb_serial_suspend(struct usb_interface *intf,
>> pm_message_t message)
>>        }
>>
>>        if (serial->type->suspend)
>> -               r = serial->type->suspend(serial, message);
>> +               return serial->type->suspend(serial, message);
>>
>> -       return r;
>> +       return 0;
>>  }
>>  EXPORT_SYMBOL(usb_serial_suspend);
>>
>>  int usb_serial_resume(struct usb_interface *intf)
>>  {
>>        struct usb_serial *serial = usb_get_intfdata(intf);
>> -       int rv;
>>
>>        serial->suspending = 0;
>> -       if (serial->type->resume)
>> -               rv = serial->type->resume(serial);
>> -       else
>> -               rv = usb_serial_generic_resume(serial);
>>
>> -       return rv;
>> +       return (serial->type->resume)
>
>   Parens totally not needed here.
>
>> +               ? serial->type->resume(serial)
>> +               : usb_serial_generic_resume(serial);
>>  }
>>  EXPORT_SYMBOL(usb_serial_resume);
>
> WBR, Sergei
> --
> 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/
>

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Greg KH]

On Fri, Jul 24, 2009 at 12:48:03PM -0300, Trevor Pace wrote:
> Removed useless return value variables.

Why are they "useless"?

They seem useful to me, especially as it causes the code to fall out of
the function at the bottom, and not in the middle, which makes
maintaining the code easier to do over time, right?

And did this actually cause any generated code to be
faster/smaller/better?

Oh, and you seem to have messed up a bit of whitespace, please always
run your patches through scripts/checkpatch.pl first.

thanks,

greg k-h

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Joe Perches]

On Fri, 2009-07-24 at 10:34 -0700, Greg KH wrote:
> On Fri, Jul 24, 2009 at 12:48:03PM -0300, Trevor Pace wrote:
> > Removed useless return value variables.
> They seem useful to me, especially as it causes the code to fall out of
> the function at the bottom, and not in the middle, which makes
> maintaining the code easier to do over time, right?

If paint for the bike shed is still required,
my preferences are:

If unwinding is required, gotos are useful
and the last return should be the error case.

If no unwinding is required, gotos tend to be
distracting and the last return should be the
no error case.

Trevor's new code does:

	if (port->serial->type->ioctl)
		return port->serial->type->ioctl(tty, file, cmd, arg);
	return -ENOIOCTLCMD;

where I would have preferred:

	if (!port->serial->type->ioctl)
		return -ENOIOCTLCMD;

	return port->serial->type->ioctl(tty, file, cmd, arg);

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Greg KH]

On Fri, Jul 24, 2009 at 11:02:22AM -0700, Joe Perches wrote:
> On Fri, 2009-07-24 at 10:34 -0700, Greg KH wrote:
> > On Fri, Jul 24, 2009 at 12:48:03PM -0300, Trevor Pace wrote:
> > > Removed useless return value variables.
> > They seem useful to me, especially as it causes the code to fall out of
> > the function at the bottom, and not in the middle, which makes
> > maintaining the code easier to do over time, right?
> 
> If paint for the bike shed is still required,

Sorry, it's not, it's staying mauve and you better like it!

:)

thanks,

greg k-h

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Greg KH]

On Fri, Jul 24, 2009 at 02:06:16PM -0300, Trevor Pace wrote:
>  int usb_serial_resume(struct usb_interface *intf)
>  {
>  	struct usb_serial *serial = usb_get_intfdata(intf);
> -	int rv;
> 
>  	serial->suspending = 0;
> -	if (serial->type->resume)
> -		rv = serial->type->resume(serial);
> -	else
> -		rv = usb_serial_generic_resume(serial);
> 
> -	return rv;
> +	return (serial->type->resume)
> +		? serial->type->resume(serial)
> +		: usb_serial_generic_resume(serial);

Sorry, but no, I'm not going to take something like this.

The first version is just so much more readable, and easier to
understand, which is the main point.  We want and need code to be easy
to read and understand at a very quick glance.  This change only makes
it harder to do so.

thanks,

greg k-h

Re: [PATCH] Removed useless retval variables in usb-serial.c

[Greg KH]

On Fri, Jul 24, 2009 at 06:53:17PM +0200, Oliver Neukum wrote:
> Am Freitag, 24. Juli 2009 17:48:03 schrieb Trevor Pace:
> > Removed useless return value variables.
> >
> > Signed-off By: Trevor Pace <trevor.pace@dal.ca>
> 
> These changes are a bad idea. They'll bite us in the ass as soon as we
> change locking or need to do more cleanups. The preferred form of
> error handling is
> 
> r = op()
> if (r < 0)
> 	goto error_exit;
> 
> I suggest that you don't apply them.

I agree with you, and will not be applying them.

thanks,

greg k-h

Fwd: [PATCH] Removed useless retval variables in usb-serial.c

[Trevor Pace]

Forgot to CC the mailing list as well on this one.

Sorry,

Trevor Pace




---------- Forwarded message ----------
From: Trevor Pace <trevorpace@gmail.com>
Date: Fri, Jul 24, 2009 at 2:47 PM
Subject: Re: [PATCH] Removed useless retval variables in usb-serial.c
To: Greg KH <gregkh@suse.de>


Hey Greg,

First off, thanks for the feedback. As you can probably tell I haven't
done much kernel patching before.

Perhaps "useless" wasn't the term I should have used. Those retval
variables seemed to just add more to the code then appears to be
necessary.  You are right though the compiler probably optimized them
right out of the code so I can't imagine a performance increase as a
result of this.

I quite like that idea of having the code fall out the bottom and I
guess I just never really thought about it that way.

Anyway thanks again,

Trevor Pace



On Fri, Jul 24, 2009 at 2:34 PM, Greg KH<gregkh@suse.de> wrote:
> On Fri, Jul 24, 2009 at 12:48:03PM -0300, Trevor Pace wrote:
>> Removed useless return value variables.
>
> Why are they "useless"?
>
> They seem useful to me, especially as it causes the code to fall out of
> the function at the bottom, and not in the middle, which makes
> maintaining the code easier to do over time, right?
>
> And did this actually cause any generated code to be
> faster/smaller/better?
>
> Oh, and you seem to have messed up a bit of whitespace, please always
> run your patches through scripts/checkpatch.pl first.
>
> thanks,
>
> greg k-h
> --
> 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/
>