
<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><div><br></div><br><div><div>On Oct 3, 2013, at 3:54 PM, Renato Golin <<a href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">On 3 October 2013 18:49, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Could you add a test case for each case you are fixing (possibly in the same test file)?<br>

</div></blockquote><div><br></div><div>Yes, that'd make a lot clearer on what kind of bug you're hitting and how you're fixing them.</div><div><br></div><div>I'd prefer a specific test on each patch, so that it gets bundled and helps future patch reviewers to understand the context.</div>

<div><br></div><div></div></div></div><div class="gmail_extra">Regarding the patches, without looking at an example, it's hard to understand what you're trying to fix. Still, the addOperand() patch seems straight-forward, and the tail call patch looks correct, though it'd be good to see it in action on a test. But it's not easy to spot what the optimizeSelect patch is trying to fix.</div>

<div class="gmail_extra"><br></div><div class="gmail_extra">Also, in addition to a test on each patch, would be good to get better commit descriptions, especially on the optimizeSelect patch.</div><div class="gmail_extra">

<br></div><div class="gmail_extra">cheers,</div><div class="gmail_extra">--renato</div></div>

</blockquote></div><br></body></html>