Hello Michael, While running a static analyzer on Inkscape's codebase [1], I found a fairly severe bug in our copy of libavoid. I checked your git repository, and the bug is still present.
In graph.cpp, from line 643:
void EdgeList::clear(void) { while (_firstEdge) { delete _firstEdge; } COLA_ASSERT(_count == 0); _lastEdge = NULL; }
The bug here is that the compiler might optimize the loop-condition and only check it once, assuming that the destructor of _firstEdge does not have the side-effect of changing _firstEdge. In fact, libavoid's design is such that the destructor does have such a side effect. I find this a *very* questionable design, that is bound to cause bugs for developers contributing code or, in this case, by a (afaik fairly common, see [2]) compiler optimization. I am surprised you have not had programs run into this problem. However, I cannot rewrite all the code that relies on that behavior. I hope you would consider redesigning the list implementation, such that it does not have unintuitive side effects.
For now, I am planning to "fix" this bug by:
void EdgeList::clear(void) { EdgeInf *edge = _firstEdge; while (edge) { EdgeInf *next_edge = edge->lstNext; delete edge; edge = next_edge; } COLA_ASSERT(_count == 0); COLA_ASSERT(_firstEdge == NULL); _lastEdge = NULL; }
Do you agree with this fix?
Thank you for your comment. Kind regards, Johan
[1] http://ec2-54-69-235-61.us-west-2.compute.amazonaws.com:8080/job/Inkscape_tr...
[2] http://stackoverflow.com/questions/9495856/how-to-prevent-g-from-optimizing-...