On Wed, Nov 17, 2010 at 12:50:10PM -0700, Pete Zaitcev wrote:
> On Wed, 17 Nov 2010 11:40:22 -0800
> Matthew Dharm <mdharm-usb@xxxxxxxxxxxxxxxxxx> wrote:
>
> > > > > + length = 0;
> > > > > + fp = urb->iso_frame_desc;
> > > > > + while (ndesc-- != 0) {
> > > > > + if (fp->actual_length != 0) {
> > > > > + if (fp->offset + fp->actual_length > length)
> > > > > + length = fp->offset + fp->actual_length;
> > > > > + }
> > > >
> > > > {} are not needed here, and thwo *if* statements can be collapsed into one.
> > >
> > > They are semantically different and thus they were left uncollaplsed.
> > > The upper one selects the descriptor and the lower own is an
> > > open-coded min. Also, any collapsing would lead to hideously ugly
> > > continuation lines.
> >
> > What about doing the internal if() with a MIN() instead? It looks
> > equivalent to
> > length = MIN(length, fp->offset + fp->actual_length);
>
> That looks good, I just keep forgetting what min and MIN do,
> and the typing restrictions... Need to read up on them.
Of course, it would be helpful if we used MAX() instead, since that's
actually what the code does.
And the very fact that I confused myself when translating this to use
MIN/MAX suggests that it needs to be re-written to be more clear.
Matt
--
Matthew Dharm Home: mdharm-usb@xxxxxxxxxxxxxxxxxx
Maintainer, Linux USB Mass Storage Driver
It's not that hard. No matter what the problem is, tell the customer
to reinstall Windows.
-- Nurse
User Friendly, 3/22/1998
Attachment:
pgpnP7FW7JtIe.pgp
Description: PGP signature
