Hi there,
[...] I believe the functions that may have side effects on their arguments should use an API that somehow reflects that. For instance:
DwExtIterator* a_Dw_ext_iterator_new (DwIterator *it);
Should be
DwExtIterator* a_Dw_ext_iterator_new (DwIterator **it); ^^
Yeah, just a typo.
Another possibility would be to define wrapper macros:
#define a_Dw_ext_iterator_new(it) _a_Dw_ext_iterator_new(&it)
DwExtIterator* _a_Dw_ext_iterator_new (DwIterator **it);
but that's a bit kludgy for my taste.
Yep.
The same applies for a_Dw_iterator_free() and friends.
It should be quite obvious that functions ending on "_free", "_destroy" etc. have a side-effect of their argument, so I'd keep this and regard it as an exception. a_Dw_ext_iterator_new is another case, and very confusing, so this one should be fixed.
Hem, that's what I meant (rush writing).
BTW, the first approach I wrote was to force a_Dw_iterator_free to make NULL its argument).
Very useful for debugging! :-)
That was the idea!
Well, that's it. As I don't know the iterators code as well as Sebastian, I'll let him make the decisions.
Well, first I'd use this change for the release, and fix it for 0.7.3. A grep for "a_Dw_ext_iterator_new" shows that this is handled correctly elsewhere.
OK guys, please give Copy&Paste some hard testing. If it shows no problems, I'll probably make the final touches this Monday and try to make a release on Tuesday.
Perhaps it is cleaner to remove the side-effect completely,
That sounds good and clean.
by letting a_Dw_ext_iterator_new putting a clone on top of the stack, so that the callers must free the simple iterator. This would be a bit slower, but compared to that, a_Dw_ext_iterator_new creates anyway quite a couple of DwIterator's. Has to be tested.
Yes, probably 0.7.3 will require some profiling and tunning.
(Thorben: As far as I can say from a first gprof run, this is not the critical code, so the problem is not made worse.)
This problem certainly is worth an investigation. Let it be on 0.7.3. Cheers Jorge.-