Changing the two loops to use the same form improves readability because it makes it clear that the form of iteration is the same between the two loops. This is a very common C pattern when dealing with lists where the behavior changes after an element is encountered. The pattern is used instead of a flag because it eliminates branches in the code. In this case, not using the canonical two-for-loop form has lead the original writer to create code that is not obviously correct - you can’t look at this function and see that it does what it claims - in fact, the most obvious code path of exiting the first loop through the while condition directly results in a null pointer dereference.
Code should communicate meaning to other software engineers. It should allow us to reason together about the code, and to have certainty about what the code will do. I don’t think anyone could look at the existing code and be assured that it was correct. Fixing that problem and clearly communicating the correctness of the code is not “code churn” - on the contrary it is the kind of change that is vital to the long-term health of the codebase. Tests are a good thing, but they are not the only thing. Consider the state-space of a large piece of software like webkit - it is essentially infinite. You can’t test every case and code path to ensure correctness. On the other hand, we don’t yet have the tools to produce a proof of correctness for webkit, and so we live somewhere between the two - some of our confidence comes from being able to reason about the correctness of the code in general, and some of our confidence comes from being able to test a small fraction of the state space. What do we hope to achieve by adding a test when fixing a bug? To prevent a bug from being reintroduced into the codebase at a later date. This is a reasonable goal, so let’s remember that the goal is to prevent the bug from recurring, not to add a test for its own sake. In this case, the potential null pointer dereference was found using coverity, a static analysis tool that we run nightly. If the bug were to be reintroduced it is reasonable to expect that static analysis would be able to find it again. Hope this helps you see where I'm coming from, Luke _______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
