On Wed, 2012-09-26 at 10:39 -0400, Mathieu Desnoyers wrote: > * Sasha Levin ([email protected]) wrote: > > On 09/26/2012 03:59 PM, Steven Rostedt wrote: > > > On Wed, 2012-09-26 at 14:45 +0100, David Laight wrote: > > >> Amazing how something simple gets lots of comments and versions :-) > > >> > > >>> ... > > >>> + * This has to be a macro since HASH_BITS() will not work on pointers > > >>> since > > >>> + * it calculates the size during preprocessing. > > >>> + */ > > >>> +#define hash_empty(hashtable) > > >>> \ > > >>> +({ > > >>> \ > > >>> + int __i; > > >>> \ > > >>> + bool __ret = true; > > >>> \ > > >>> + > > >>> \ > > >>> + for (__i = 0; __i < HASH_SIZE(hashtable); __i++) > > >>> \ > > >>> + if (!hlist_empty(&hashtable[__i])) > > >>> \ > > >>> + __ret = false; > > >>> \ > > >>> + > > >>> \ > > >>> + __ret; > > >>> \ > > >>> +}) > > >> > > >> Actually you could have a #define that calls a function > > >> passing in the address and size. > > > > > > Probably would be cleaner to do so. > > > > I think it's worth it if it was more complex than a simple loop. We > > were doing a similar thing with the _size() functions (see version 4 > > of this patch), but decided to remove it since it was becoming too > > complex. > > Defining local variables within statement-expressions can have some > unexpected side-effects if the "caller" which embeds the macro use the > same variable name. See rcu_dereference() as an example (Paul uses an > awefully large number of underscores). It should be avoided whenever > possible. > > > > > > > > > >> Also, should the loop have a 'break' in it? > > > > > > Yeah it should, and could do: > > > > > > for (i = 0; i < HASH_SIZE(hashtable); i++) > > > if (!hlist_empty(&hashtable[i])) > > > break; > > > > > > return i < HASH_SIZE(hashtable); > > > Hrm, Steven, did you drink you morning coffee before writing this ? ;-) > It looks like you did 2 bugs in 4 LOC.
Coffee yes, but head cold as well. :-p > > First, the condition should be reversed, because this function returns > whether the hash is empty, not the other way around. Bah, I was looking at the code the code and got the ret confused. I originally had it the opposite, and then reversed it before sending. > > And even then, if we would do: > > for (i = 0; i < HASH_SIZE(hashtable); i++) > if (!hlist_empty(&hashtable[i])) > break; > > return i >= HASH_SIZE(hashtable); > > What happens if the last entry of the table is non-empty ? It still works, as 'i' is not incremented due to the break. And i will still be less than HASH_SIZE(hashtable). Did you have *your* cup of coffee today? ;-) > > So I would advise that Sasha keep his original flag-based > implementation, but add the missing break, and move the init and empty > define loops into static inlines. > Nah, -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
