On Thu, Dec 16, 2004 at 05:41:32AM -0400, bulia byak wrote:
crashes now. dir-utils.cpp:30 is dying from a NULL value for 'base.'
Latest CVS still craches on bitmap import. Looks like the person who touched this code recently was pjrm. Peter, could you please look into this? And please test your changes thoroughly before committing. (Of course errors happen, but this is a very obvious bug which should have been caught before committing).
I did indeed test before committing, both by using inkscape and ensuring that the changed code was executed, and even by creating a new unit test program (dir-util-test.cpp). To detect this bug, it isn't sufficient to test the changed code or even to test every caller of the changed code. I suggest that this bug is not caused by inappropriately little testing by the developer.
What then can we learn from this bug in terms of how to change our development practice?
One response would be: "The bug has been found in the normal course of use, and is believed now fixed. We needn't change our development practice."
Another response would be to encourage greater peer review. For this bug, a reviewer of the diff might have noticed the change in behaviour for base==NULL, and might question whether this change affected any callers.
The cost would be more time spent reviewing instead of developing, and more time spent responding to reviewer suggestions. In one project I know of (developing a compiler, so they have higher quality obligations than inkscape does), they require all non-trivial diffs to be posted to a mailing list and reviewed & OK'd by someone before committing. (Some simple/important changes can be committed before being OK'd, but still require posting to the mailing list.) If we do go with the `review more' approach, then I suggest that we start much lower key, merely encouraging ppl to do `cvs -q diff -dpU5 -r BASE -r HEAD' and read the diffs.
Incidentally, please compile your changes before committing, and look for any introduced warnings. E.g. this would have alerted Bulia to a bug he introduced a few days ago which I've now fixed.
Btw, I won't be around for the next week or two. Visiting my parents in the country.
pjrm.