lkml.org 
[lkml]   [2006]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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/

\
 
 \ /
  Last update: 2006-03-20 07:15    [W:0.070 / U:1.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site