[dm-devel] [PATCH v3 01/17] hashtable: introduce a small and naive hashtable

[Pedro Alves]

On 09/04/2012 04:35 PM, Steven Rostedt wrote:
> On Tue, 2012-08-28 at 19:00 -0400, Mathieu Desnoyers wrote:
> 
>> 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.

-- 
Pedro Alves