On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum <span dir="ltr"><<a href="mailto:groby@chromium.org">groby@chromium.org</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="gmail_quote"><div>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. <span style="font-family:arial,sans-serif;line-height:18px">And most importantly, these changes are clearly not "code cleanup".</span></div>




</div></div></blockquote><div><br></div></div><div>I'm disagreeing here. (as far as NULL checks go).</div><div><br></div><div>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.</div>

</div></blockquote><div><br></div><div>The burden of proof of correctness is always on the patch author in WebKit. It doesn't matter whether the patch is a simple cleanup, adding a new feature, or fixing a bug.</div>
<div>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>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 :)</div>




<div><br></div><div>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. </div>

</div></blockquote><div><br></div><div>As multiple reviewers have repeatedly stated in this thread, I don't think "cleanup only" is an acceptable description of a change of this nature. "cleanup only" implies that there is no behavioral change.</div>

<div><br></div><div>If we're modifying the code to make WebKit more deterministic, then there is an observable behavior change, namely that WebKit is more deterministic than it used to be. And in this particular case, WebKit may even have one less crash bug. Such a behavioral change should ideally be tested. If it cannot be tested or that coming up with a test case is too hard, then it should be explained in the change log, and I don't consider "cleanup" as a satisfactory explanation.</div>

<div><br></div><div>- Ryosuke</div><div><br></div></div>