[webkit-dev] Eliminate potential null pointer dereference?

[Filip Pizlo]

On Apr 20, 2012, at 11:40 AM, Benjamin Poulain <benjamin at webkit.org> wrote:

> On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
> <macpherson at chromium.org> wrote:
>> 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.
> 
> I remember seeing similar patch based on Coverty, and when asked for a
> test, the author discovered the null check was useless in that place
> and the code was changed differently.
> 
> I wholeheartedly agree we should try to make tests for code changed
> based on static code analyzer. This will help making sure the change
> is meaningful, and the test would prevent regression.

+1

Someone in this thread said something about code readability. So let's consider that. If I see code like:

if (!var) thingy();

Then I will be under the impression that var might sometimes be zero and that thingy() might sometimes happen, and that the previous webkittens to touch this code had a good reason for this check. 

Coverity is no more accurate than testing; if it tells you that var might be zero than it cannot, will not, and does not give you 100% confidence that this is reachable. 

Hence if you add special cases for things that coverity warned you about, you are potentially doing a disservice to anyone looking at the code in the future. You are telling them that "var might be zero" when really you meant to say "I put in this check to get my tool to stop complaining". 

On the other hand, if you are able to construct a test that demonstrates reachability then you win. But if there is no test then see my previous paragraph. 

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