* Pedro Alves (palves-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote:

On 09/04/2012 05:30 PM, Pedro Alves wrote:

quoted

On 09/04/2012 04:35 PM, Steven Rostedt wrote:

quoted

On Tue, 2012-08-28 at 19:00 -0400, Mathieu Desnoyers wrote:

quoted

Looking again at:

+#define hash_for_each_size(name, bits, bkt, node, obj, member)                 \
+       for (bkt = 0; bkt < HASH_SIZE(bits); bkt++)                             \
+               hlist_for_each_entry(obj, node, &name[bkt], member)

you will notice that a "break" or "continue" in the inner loop will not
affect the outer loop, which is certainly not what the programmer would
expect!

I advise strongly against creating such error-prone construct.

A few existing loop macros do this. But they require a do { } while ()
approach, and all have a comment.

It's used by do_each_thread() in sched.h and ftrace does this as well.
Look at kernel/trace/ftrace.c at do_for_each_ftrace_rec().

Yes it breaks 'break' but it does not break 'continue' as it would just
go to the next item that would have been found (like a normal for
would).

/*
 * This is a double for. Do not use 'break' to break out of the loop,
 * you must use a goto.
 */
#define do_for_each_ftrace_rec(pg, rec)                                 \
        for (pg = ftrace_pages_start; pg; pg = pg->next) {              \
                int _____i;                                             \
                for (_____i = 0; _____i < pg->index; _____i++) {        \
                        rec = &pg->records[_____i];



You can make 'break' also work as expected if you can embed a little knowledge
of the inner loop's condition in the outer loop's condition.  Sometimes it's
trivial, most often when the inner loop's iterator is a pointer that goes
NULL at the end, but other times not so much.  Something like (completely untested):

#define do_for_each_ftrace_rec(pg, rec)                                 \
        for (pg = ftrace_pages_start, rec = &pg->records[pg->index];    \
             pg && rec == &pg->records[pg->index];                      \
             pg = pg->next) {                                           \
                int _____i;                                             \
                for (_____i = 0; _____i < pg->index; _____i++) {        \
                        rec = &pg->records[_____i];


(other variants possible)

IOW, the outer loop only iterates if the inner loop completes.  If there's
a break in the inner loop, then the outer loop breaks too.  Of course, it
all depends on whether the generated code looks sane or hideous, if
the uses of the macro care for it over bug avoidance.

BTW, you can also go a step further and remove the need to close with double }},
with something like:

#define do_for_each_ftrace_rec(pg, rec)                                          \
        for (pg = ftrace_pages_start, rec = &pg->records[pg->index];             \
             pg && rec == &pg->records[pg->index];                               \
             pg = pg->next)                                                      \
          for (rec = pg->records; rec < &pg->records[pg->index]; rec++)

Maybe in some cases there might be ways to combine the two loops into
one ? I'm not seeing exactly how to do it for this one, but it should
not be impossible. If the inner loop condition can be moved to the outer
loop, and if we use (blah ? loop1_conf : loop2_cond) to test for
different conditions depending on the context, and do the same for the
3rd argument of the for() loop. The details elude me for now though, so
maybe it's complete non-sense ;)

It might not be that useful for do_for_each_ftrace_rec, but if we can do
it for the hash table iterator, it might be worth it.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com