[webkit-dev] Eliminate potential null pointer dereference?

[Rachel Blum]

>
> I completely agree with Maciej here that if this is a reachable code, then
> the patch author should put a reasonable effort into creating a test case. And
> most importantly, these changes are clearly not "code cleanup".
>

I'm disagreeing here. (as far as NULL checks go).

Unless there's a demonstrable reason that you _need_ a value uninitialized,
why is the burden of proof on the person doing cleanup? Yes, at the point
the code was written, it's well possible that the author was aware that the
value would always be initialized for use. However, if code is added to a
class, that invariant is not always checked again.

I think the confusion is over the intent of the person making the cleanup
change. We (I speak as one of the people pushing static analysis) are not
interested in *changing* WebKit behavior. We're interested in making sure
behavior is deterministic. Requiring the construction of what amounts to an
exploit for each fix for uninitialized variables seems a bit overkill :)

I agree that the CHANGELOG entry should state that we deliberately didn't
add tests. My personal policy is to propose those patches, complete with
"No new tests/ cleanup only". If I get pushback on the review, I'm happy to
abandon it.

Rachel


> On Thu, Apr 19, 2012 at 11:11 PM, David Levin <levin at google.com> wrote:
>
>> 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.
>
>
> The WebKit contribution guide lists this as a requirement (
> http://www.webkit.org/coding/contributing.html):
> For any feature that affects the layout engine, a new regression test must
> be constructed. If you provide a patch that fixes a bug, that patch should
> also include the addition of a regression test that would fail without the
> patch and succeed with the patch. If no regression test is provided, the
> reviewer will ask you to revise the patch, so you can save time by
> constructing the test up front and making sure it's attached to the bug.
>
> So I don't think we can "disagree" on this topic. I'm sympathetic to the
> argument that it's hard to come up with a test case for things like this,
> but then the patch author should clearly state that in the change log, and
> most importantly the reviewer should be asking that during the review.
>
> - Ryosuke
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120420/f0be6406/attachment.html>