Messages in this thread | | | Date | Mon, 20 Mar 2006 07:12:13 +0100 | From | Willy Tarreau <> | Subject | Re: OOPS: 2.6.16-rc6 cpufreq_conservative |
| |
On Sun, Mar 19, 2006 at 11:33:17AM -0800, Linus Torvalds wrote:
> Of course, I shouldn't say "works", since it is still totally untested. It > _looks_ good, and that statement expression usage is just _so_ ugly it's > cute. > > Linus
At least, you could have moved the macro closer to where it's used. It's very uncommon to break a statement within an if condition, and it's not expected that the macro you're calling does a break under you. It took me several minutes to understand precisely how this works. Now it seems trivial, but I guess that at 3am I would have gone to bed instead.
One first enhancement would be to make it easier to understand by putting it closer to its user :
#elif NR_CPUS > 1 #define check_for_each_cpu(cpu, mask) \ ({ unsigned long __bits = (mask).bits[0] >> (cpu); if (!__bits) break; __bits & 1; })
#define for_each_cpu_mask(cpu, mask) \ for ((cpu) = 0; (cpu) < NR_CPUS; (cpu)++) \ if (!check_for_each_cpu(cpu, mask)) \ continue; \ else
Now, does removing the macro completely change the output code ? I think that if something written like this produces the same code, it would be easier to read :
#define for_each_cpu_mask(cpu, mask) \ for ((cpu) = 0; (cpu) < NR_CPUS; (cpu)++) { \ unsigned long __bits = (mask).bits[0] >> (cpu); \ if (!__bits) \ break; \ if (!__bits & 1) \ continue; \ else
Regards, Willy
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |