[llvm-commits] [llvm] r167740 - in /llvm/trunk: lib/Transforms/Scalar/Reassociate.cpp test/Transforms/Reassociate/mul_neg.ll test/Transforms/Reassociate/multistep.ll

[Chandler Carruth]

On Mon, Nov 12, 2012 at 7:12 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>
> On Nov 12, 2012, at 7:00 PM, Chandler Carruth <chandlerc at google.com> wrote:
>
>> On Mon, Nov 12, 2012 at 6:55 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>
>>> On Nov 12, 2012, at 3:51 PM, Chris Lattner <clattner at apple.com> wrote:
>>>
>>>>
>>>> On Nov 12, 2012, at 1:08 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>>
>>>>> Hi Eli,
>>>>>
>>>>> The approach seems sound to me. Since no one has raised design concerns at this point and the fact this is a a bug fix for a crash, I think it's appropriate for review after commit.
>>>>
>>>> This patch isn't "obvious", and Shuxin is still new to this part of the code.  Unless it's been reviewed it shouldn't go in.
>>>
>>> The patch was submitted for review back on 11/5 and there were some discussions on 11/6 and 11/7. There were no additional comments since then. Should it continue to sit in review limbo?
>>
>> The standard response, and what everyone else in this position does
>> (and there are *many*...) is to ping, asking for an update.
>
> Which is why I said the approach seems fine to me (and I looked at the patch and commented on it back on 11/6). Since no one has any concrete suggestion it can be taken as LGTM. If anyone has other suggestions, they can be done after commit.

I had concrete concerns (expressed in the code review) and suggestions
expressed both in my review, and in person to Shuxin during the dev
meeting. The worklist approach seems not yet correct to me. I tried to
explain this both in the review thread and when talking in person
during the hacking session at the dev meeting and tried to indicate
that a follow-up email would be fine to further clarify. I was waiting
to hear back if that had failed to make sense.

All this said, the correct approach is to move this discussion to the
code review thread until there is some consensus there.