Messages in this thread | | | Date | Wed, 26 Sep 2012 10:39:05 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH v6] hashtable: introduce a small and naive hashtable |
| |
* Sasha Levin (levinsasha928@gmail.com) 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.
First, the condition should be reversed, because this function returns whether the hash is empty, not the other way around.
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 ?
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.
Thanks,
Mathieu
> > Right. > > > Thanks, > Sasha
-- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
| |