FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 09-04-2012, 11:27 PM
Steven Rostedt
 
Default hashtable: introduce a small and naive hashtable

On Tue, 2012-09-04 at 23:58 +0100, Pedro Alves wrote:

> Not true. The comma operator introduces a sequence point. It's the comma
> that separates function parameters that doesn't guarantee ordering.

Bah! C language is almost as confusing as English.

-- Steve



--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 03:11 PM
Steven Rostedt
 
Default hashtable: introduce a small and naive hashtable

On Thu, 2012-09-06 at 07:55 -0700, Josh Triplett wrote:

> > My solution to making 'break' work in the iterator is:
> >
> > for (bkt = 0, node = NULL; bkt < HASH_SIZE(name) && node == NULL; bkt++)
> > hlist_for_each_entry(obj, node, &name[bkt], member)
> >
>
> Looks reasonable. However, it would break (or rather, not break) on
> code like this:
>
> hash_for_each_entry(...) {
> if (...) {
> foo(node);
> node = NULL;
> break;
> }
> }
>
> Hiding the double loop still seems error-prone.

We've already had this conversation ;-) A guess a big comment is in
order:

/*
* NOTE! Although this is a double loop, 'break' still works because of
* the 'node == NULL' condition in the outer loop. On break of
* the inner loop, node will be !NULL, and the outer loop will
* exit as well.
*/

-- Steve


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 04:00 PM
Steven Rostedt
 
Default hashtable: introduce a small and naive hashtable

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


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 05:15 PM
Steven Rostedt
 
Default hashtable: introduce a small and naive hashtable

On Thu, 2012-09-06 at 18:21 +0200, Sasha Levin wrote:
> On 09/06/2012 06:00 PM, Steven Rostedt wrote:
> >> > 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 :-)
>
> While we're on this subject, I haven't actually seen hlist_for_each_entry() code
> that even *touches* 'pos'.
>
> Will people yell at me loudly if I change the prototype of those macros to be:
>
> hlist_for_each_entry(tpos, head, member)
>
> (Dropping the 'pos' parameter), and updating anything that calls those macros to
> drop it as well?

If 'pos' is no longer used in the macro, I don't see any reason for
keeping it around.

-- Steve


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 02:42 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org