On Fri, Apr 25, Jorge Arellano Cid wrote:
After some hard code revisions, I decided to "eat" Valgrind's manual, install it and BINGO!:
[...]
What does this mean?
As a_Dw_ext_iterator_new(it) may _free_ 'it', 'it' should no longer be used after that call. That was the case with the page (right-click test case).
I commited a short _code_change_ that avoids the problem. It is not a "patch" but just illustrative code.
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); ^^
and be used like:
a_Dw_ext_iterator_new(&it);
so 'it' can be NULL at return time.
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.
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.
BTW, the first approach I wrote was to force a_Dw_iterator_free to make NULL its argument).
Very useful for debugging! :-)
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. Perhaps it is cleaner to remove the side-effect completely, 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. (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.) Sebastian