On Wed, Feb 09, 2005 at 11:33:49AM -0800, David Brownell wrote:
> On Wednesday 09 February 2005 11:01 am, Nishanth Aravamudan wrote:
> > On Wed, Feb 09, 2005 at 10:33:02AM -0800, David Brownell wrote:
> > > On Tuesday 08 February 2005 10:05 pm, Nishanth Aravamudan wrote:
> > > > - USB_REQ_CLEAR_FEATURE, USB_RT_HUB, feature, 0, NULL, 0,
> > > > HZ);
> > > > + USB_REQ_CLEAR_FEATURE, USB_RT_HUB, feature, 0, NULL, 0,
> > > > 1000);
> > >
> > > You know, changing from symbols to magic numbers is not a win.
> > > People know what something involving HZ is intended to mean,
> > > but 1000 is just another magic number ...
> >
> > To be honest, I had a similar concern. But if the API is clear that the
> > timeout parameter is in real time units (milliseconds, in this case), I
> > do not think 1000 is as much of a "magic number" as HZ was, or
> > minimally, it's the same.
>
> You mean you can actually tell, from looking at a large set of
> function parameters, which ones indicate timeouts? Wow! Most folk
> I know usually deduce that it's the one that involves HZ, MSEC, or
> USEC ... or, it's the single parameter to a *sleep function. :)
I don't want to generalize, but it seems like there is an implicit
attempt to always make the timeout parameter (if there is one) be the
last parameter to most functions. Regardless, USB is nicely commented
(at least usb_{control,bulk}_msg() indicating that the (after my patch)
the timeout is in msecs. I figure anyone using/reading code will go for
the definition of called functions first and then try to understand
parameters...
> For maintainability, code needs to have a few basic accommodations
> for newcomers -- or semi-forgetful oldtimers. (I think that covers
> pretty much every developer, come to think of it...) One of those
> accomodations is using symbolic constants.
Being a newbie myself, I certainly understand that. But given the
complexity of the kernel, and the fact that some functions (even
usb_start_wait_urb(), in this case) are defined multiple times, I always
go for the definition first, just to see if there are any special
comments (which there are in this case). While it is somewhat slower
than reading the code directly (via your MS_PER_MSEC option), it is
about the same, to me, as translating HZ-related values to real time
units.
> > > Rather than 1000 how about something like "1 * MS_PER_SEC" or something
> > > similar? I especially don't like to see magic numbers added to usbcore,
> > > but the same issue applies to a lot of drivers too.
> >
> > *sigh* That would mean sending out all ~63 patches again, but so it
> > goes. If Greg would prefer it this way, I'd be happy to comply.
>
> One common practice is to send out core patches plus a few
> example "here's how the drivers would change" patches, purely
> for comments. (Especially if, as you say, you have concerns;
> they might be shared by other folk.) Then wait a few days
> (idealy a week or so) to collect feedback before submitting
> the patches.
>
> Whenever you send out a large batch of patches, or a set of
> related complex patched, it's best to have at least tried to
> collect feedback beforehand ... to avoid exactly that problem.
Thanks for the suggestions! It's all part of the learning process for me
and I will make sure to do what you suggested with my next such set of
patches.
Thanks,
Nish
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
