On Fri, 25 Apr 2014, Dan Williams wrote:
> > Here's a second question. I don't know if there's any definitive
> > answer. What do you think of passing values like hdev, hcd, and
> > port_dev as arguments, as opposed to re-deriving them from the other
> > values?
> >
> > In theory, it could result in slightly smaller object code,
> > particularly in cases (like here) where the whole routine can be
> > inlined. Also, it might reduce slightly the chances for copy/pasting
> > errors.
> >
> > This isn't a big deal either way. But people seem to prefer passing
> > fewer arguments, and I have often wondered why.
>
> 80 columns?
Maybe.
> We could do something like:
>
> struct usb_port_context {
> struct usb_port port_dev;
> struct usb_hub *hub;
> struct usb_device *hdev;
> int port1;
> u16 portstatus, portchange;
> }
>
> ...and pass that around. But I think that is a cleanup for another
> patch series.
That doesn't seem like a big improvement. Why go to all the trouble of
creating a data structure to hold your local variables when you can
just pass them on the stack? In the case where you're calling an
inline routine, the compiler often wouldn't even need to allocate new
stack locations to hold these values; it could reuse the local
variables in the calling function.
> >> + if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange))
> >> + connect_change = 1;
> >
> > Moving this little portion is a candidate for the cleanup patch.
>
> Moving it where? Sorry I've cache flushed the context for this bit.
I've completely forgotten. Maybe I was thinking ahead to a later patch
in the series.
> Attached for review, but will be resubmitted with a refresh the whole
> patch set
The file you attached was a copy of patch 12/16, not an updated version
of this patch. I'll wait for the whole patch set to reappear. (And
that reminds me, I still need to review patches 14-16 of the original
series...)
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
