[PATCH] Fix illegal DAG produced by SelectionDAG::getConstant() for v2i64 type

[David Blaikie]

On Thu, Nov 14, 2013 at 8:00 AM, Daniel Sanders
<Daniel.Sanders at imgtec.com>wrote:

>  In that case it sounds like I should be committing without needing a
> further LGTM. Thanks.
>
>
>
> 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.
>

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.

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.


> 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.
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* 13 November 2013 17:19
> *To:* reviews+D1973+public+5bd1dfc967a43b3a at llvm-reviews.chandlerc.com
> *Cc:* Benjamin Kramer; Daniel Sanders; llvm-commits at cs.uiuc.edu
>
> *Subject:* Re: [PATCH] Fix illegal DAG produced by
> SelectionDAG::getConstant() for v2i64 type
>
>
>
>
>
>
>
> On Wed, Nov 13, 2013 at 4:03 AM, Daniel Sanders <daniel.sanders at imgtec.com>
> wrote:
>
>
>   Can I check a matter of policy?
>
>   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?
>
>
>
> 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)
>
>
>
>
>   I don't mind either way. I just want to double-check what I should be
> doing for this patch.
>
>   Thanks
>
>
> http://llvm-reviews.chandlerc.com/D1973
>
> BRANCH
>   msa/bugfix-getconstant2
>
> ARCANIST PROJECT
>   llvm
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131114/249f2d6d/attachment.html>