Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 2012-09-06 at 17:49 +0200, Sasha Levin wrote:
>  
> > Looks reasonable.  However, it would break (or rather, not break) on
> > code like this:
> > 
> > 	hash_for_each_entry(...) {
> > 		if (...) {
> > 			foo(node);
> > 			node = NULL;

ug, I didn't even notice this. Ignore my last email :-p

/me needs to wake-up a bit more.

> > 			break;
> > 		}
> > 	}
> > 
> > Hiding the double loop still seems error-prone.
> 
> I think that that code doesn't make sense. The users of hlist_for_each_* aren't
> supposed to be changing the loop cursor.

I totally agree. Modifying the 'node' pointer is just asking for issues.
Yes that is error prone, but not due to the double loop. It's due to the
modifying of the node pointer that is used internally by the loop
counter. Don't do that :-)


> 
> We have three options here:
> 
>  1. Stuff everything into a single for(). While not too difficult, it will make
> the readability of the code difficult as it will force us to abandon using
> hlist_for_each_* macros.
> 
>  2. Over-complicate everything, and check for 'node == NULL && obj &&
> obj->member.next == NULL' instead. That one will fail only if the user has
> specifically set the object as the last object in the list and the node as NULL.
> 
>  3. Use 2 loops which might not work properly if the user does something odd,
> with a big fat warning above them.
> 
> 
> To sum it up, I'd rather go with 3 and let anyone who does things he shouldn't
> be doing break.

I agree, the double loop itself is not error prone. If you are modifying
'node' you had better know what the hell you are doing.

Actually, it may be something that is legitimate. That is, if you want
to skip to the next bucket, just set node to NULL and do the break (as
Josh had done). This would break if the macro loop changed later on, but
hey, like I said, it's error prone ;-) If you really want to do that,
then hand coding the double loop would be a better bet. IOW, don't use
the macro loop.

-- Steve