<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Nov 14, 2013 at 8:00 AM, Daniel Sanders <span dir="ltr"><<a href="mailto:Daniel.Sanders@imgtec.com" target="_blank">Daniel.Sanders@imgtec.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">In that case it sounds like I should be committing without needing a further LGTM. Thanks.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">The reason I wasn't sure was that in this case, making the requested change ended up requiring more than just the simple change that was requested. </span></p>
</div></div></blockquote><div><br></div><div>Fair enough - if the fallout from a request like that seems like substantially more than the reviewer had in mind, such that your change seems out of scope for the approval, it's not unreasonable to request re-approval at your discretion. We're only human and often have great ideas that sound good on paper/email, but when someone actually goes to implement the change it comes out differently.<br>
<br>Use your best judgment. Though in practice at least when first getting involved in the project, I tend to err on the side of asking for approval when it's at all unclear whether approval applies/was given. It can help foster a more positive attitude (that you're not trying to rush patches in half-done, etc - which it certainly doesn't seem like you are), when applied within reason.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Adding the 'virtual' keyword
revealed that some of the new tests were incorrectly passing in the original patch. In fixing this bug, I ended up deleting the call to the function I was asked to change (and then deleted the function itself because it was now unused and the original patch
added it). The patch is still largely the same in all important respects. The only code-path that changed handles a MIPS-specific case.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><u></u> <u></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> 13 November 2013 17:19<br>
<b>To:</b> <a href="mailto:reviews%2BD1973%2Bpublic%2B5bd1dfc967a43b3a@llvm-reviews.chandlerc.com" target="_blank">reviews+D1973+public+5bd1dfc967a43b3a@llvm-reviews.chandlerc.com</a><br>
<b>Cc:</b> Benjamin Kramer; Daniel Sanders; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a></span></p><div class="im"><br>
<b>Subject:</b> Re: [PATCH] Fix illegal DAG produced by SelectionDAG::getConstant() for v2i64 type<u></u><u></u></div><p></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Wed, Nov 13, 2013 at 4:03 AM, Daniel Sanders <<a href="mailto:daniel.sanders@imgtec.com" target="_blank">daniel.sanders@imgtec.com</a>> wrote:<u></u><u></u></p><div><div class="h5">
<p class="MsoNormal"><br>
Can I check a matter of policy?<br>
<br>
Is it ok to commit the version of the patch you LGTM'd and then fix the MIPS-specific false passes (caused by the missing 'virtual' keyword) under the 'obvious change' rule? or should I wait for an explicit LGTM on the updated patch?<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Generally if someone says "LGTM as long as you do blah" you're entrusted to make those changes in your current patch and commit in one go without further review. If for some reason you got the requested changes wrong, they'll generally
be addressed in post-commit review (which, if the wrongness was sufficiently severe, could include reverting, but that's usually uncommon/unlikely as such extra changes are usually simple/mechanical in nature)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm">
<p class="MsoNormal"><br>
I don't mind either way. I just want to double-check what I should be doing for this patch.<br>
<br>
Thanks<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><br>
<a href="http://llvm-reviews.chandlerc.com/D1973" target="_blank">http://llvm-reviews.chandlerc.com/D1973</a><br>
<br>
BRANCH<br>
msa/bugfix-getconstant2<br>
<br>
ARCANIST PROJECT<br>
llvm<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><u></u><u></u></p>
</div>
</div>
</blockquote>
</div></div></div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
</div>
</div>
</blockquote></div><br></div></div>