RE: The iterator interface

[<Dirk-Jan Binnema nokia com>]

Hi Philip,

>-----Original Message-----
>From: ext Philip Van Hoof [mailto:spam pvanhoof be] 
>Sent: Tuesday, August 08, 2006 16:17
>To: Binnema Dirk-Jan (Nokia-M/Helsinki)
>Cc: tinymail-devel-list gnome org
>Subject: RE: The iterator interface
>
>On Tue, 2006-08-08 at 11:29 +0300, Dirk-Jan Binnema nokia com wrote:
>
>> >Now, who's going to write the patch? :). I can't. Mickey Mouse 
>> >doesn't allow me.
>> 
>> You mean the patch to make most iterators ops void, and the unref 
>> thingy? I can do that. Will be tonight though.
>
>Also fix all existing code of course :-)

Well, the tinymail code and my code... 

>There's one exception, and that is the build-in iterator fot 
>TnyHeaderListModel. Adding unreffing and reffing there isn't 
>needed as all item-access is locked already anyway.
>
>And adding the ref and unref would slowdown sorting. That one 
>uses 'special' _nl functions anyway (so the normal _current 
>implementation of that iterator should indeed return with an 
>extra ref, the _nl one shouldn't).

Hmmmm... I would say that *all* the _current functions should behave
the same. They're implementing the same interface after all.

As the freeing (or not) of returned values is such an error-prone
area anyway, having one of the interface implementations behave
different from all the others is asking for problems.

I think that (as mentioned before) simply adding 
	tny_iterator_iface_current_unowned () 
(as an alternative to the boolean flag mentioned before)
would be much better. That's only one extra function.

>ps. _nl stands for "no lock", because the locking happens at a 
>higher level (in the model implementation). Some are by the 
>way implemented in the model implementation and are marked as inline.

BTW, making these functions void, may help for something else
as well - thread safety; in tny_header_list_iterator:

(lock)
me->current = ....
(unlock)

return me->current->data;

Now, me->current could become NULL just after the lock? 
(Thread 2 acquires lock, sets me->current to NULL...)

We would we need something like:

(lock)
my_current = me->current = ....
(unlock)

return my_current->data;

(And that will still go wrong when my_current->data is unreffed'd 
in the meantime....)

So making 'm void safes some thread-safety issues.

tny_header_list_iterator_current does it correctly, as long as
we assume nobody is unreffing the retval while it's being used.

Best wishes,
Dirk.