On Sat, Sep 24, 2011 at 04:57:13AM +0300, Dan Carpenter wrote: > What I'm trying to explain is that kernel style is minimalist. Focus > on writing the simplest, most readable code you can. Kernel coders > are 10 times more likely to use "int i" than they are to use > "unsigned int i". > > $ git grep " int i;" | wc -l > 13956 > $ git grep " unsigned int i;" | wc -l > 1508 > > "int" is the default number in C and "unsigned int" has a modifier > and so it's a tiny bit special. When you use "unsigned int" you're > drawing attention to your variable because it is not just a normal > number. Someone reviewing that code immediately thinks, "How high > is NVEC_POOL_SIZE exactly?" Certainly that was my thought when I > saw it declared as a size_t. That's interesting. For me it's the opposite, when I see someone counting the length of an array in something different than size_t, I wonder why they're doing it; as size_t is the type specifically meaning size or count of in-memory objects (in the kernel case, that was kfifo I wondered about, or most kernel code in general) -- Likewise, why there is no irqflags_t type, that would have prevented various bugs that occured in the past when people used unsigned int instead of unsigned long, and would make it clear that the variable stores those flags, not random flags for other stuff. The size_t was not really intended to get here anyway after I noticed that, but I somehow forgot it then. > But in fact the for loop is a boring ordinary for loop. You've > tricked the reviewers, and you've distracted them from the 5 second > sleep elsewhere in your code. A 5 second timeout, and that's mostly not reached. 5 seconds seems to long, but I have not rewritten that part of the code yet (I once tried 100 ms, but that did not work and in the end just caused the EC to not respond anymore). The workers also have other bugs: (a) If I queue two writes, I queue two workers, the first one then processes two writes, and the second just burns cpu cycles for nothing (b) Event processing is mixed with general response processing which can lead to sticky keys, as the release event gets delayed I'd like to rework them at some point, but for now I kept them as they were in Marc's patch from last month. I can't do everything in a single week. > > Obviously, I'm exaggerating a bit and in this case it doesn't matter > one way or the other. Go ahead and declare it as unsigned so long > as you understand about kernel style. Don't be clever, fancy or > special. Don't optimize. Don't stand out. And don't do more work > than you have to. I also exaggerated a bit, probably because it was already past midnight. I shouldn't write emails past midnight. -- Julian Andres Klode - Debian Developer, Ubuntu Member See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/. _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
