Removed useless return value variables.
Signed-off By: Trevor Pace <[email protected]>
================================================================================
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);
Hello.
Trevor Pace wrote:
> Removed useless return value variables.
Are you sure gcc doesn't optimize them away? :-)
> Signed-off By: Trevor Pace <[email protected]>
> ================================================================================
> 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
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 <[email protected]>
================================================================================
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<[email protected]> wrote:
> Hello.
>
> Trevor Pace wrote:
>
>> Removed useless return value variables.
>
> ? Are you sure gcc doesn't optimize them away? :-)
>
>> Signed-off By: Trevor Pace <[email protected]>
>
>>
>> ================================================================================
>
>> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
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
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);
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
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
Am Freitag, 24. Juli 2009 17:48:03 schrieb Trevor Pace:
> Removed useless return value variables.
>
> Signed-off By: Trevor Pace <[email protected]>
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
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 <[email protected]>
>
> 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