[webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 19, 2012, at 11:11 PM, David Levin wrote:

> I think this all started with a lot of effort put into fixing an issue reported by a user where they said "the most popular online forum in Malaysia is broken." Then folks had to do a lot of builds (bisecting) to track down where the problem was introduced. Then they had to figure out what had broken, etc.
> 
> It was mentioned (by groby at chromium.org) that this very issue had already been flagged by own internal runs of coverity on chromium (including webkit). Now, it seemed a shame that we knew about issues in WebKit and were just ignoring them. It would be nice to be able to catch these issues faster rather than wait for a user to report it, etc. which makes the expense overall go up.
> 
> So I believe there has been some effort invested in fixing some issues pointed out by coverity which is what these changes are and I believe coverity is mentioned in other changes of this sort.
> 
> I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree.
> 
> oth, regarding the style of this particular change, I find it unusual as well.

If the change was attempting to fix an issue found by a specific static analyzer, then it should say so in the ChangeLog instead of "No new tests / code cleanup only". That goes double if a lot of such changes are being made, or people not in the know will be really confused (I don't think Alexey was the only one).

Also, the fact that it was found by a static analyzer does not exempt the contributor from making a reasonable effort to create a test case. We do accept that sometimes it's not practical and in that case it's ok to mention in the ChangeLog why it was not possible to make a test case. However, "code cleanup only" is not a reason that applies to changes made with the intent of producing a behavior change.

If we had a static analyzer that ran automatically as part of the WebKit development process, and a shared goal to get its complaints down to 0, then it might be reasonable to skip creating tests for issues that it diagnoses. But that doesn't seem to be the situation here.

Regards,
Maciej


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120420/646da008/attachment.html>