[lld] r193585 - [PassManager] add ReaderWriter{Native, YAML} to the Driver.

[Rui Ueyama]

On Tue, Oct 29, 2013 at 4:50 PM, Shankar Easwaran
<shankare at codeaurora.org>wrote:

> On 10/29/2013 6:18 PM, Chandler Carruth wrote:
>
>> No, phabricator is a tool. It has nothing to do with the policy.
>>
>> The policy is very, very simple: once you ask for a code review, wait for
>> an explicit 'LGTM' to commit.
>>
> Now I am confused, whats the policy on code thats committed without
> reviews ? What I see is we follow a post-commit review culture.
>
>  2) A discussion about a potential strategy for that patch to go in before
>> each flavor was fixed to work with this specific behavior.
>>
>> #2 was not a code review, it was just a technical idea and discussion (and
>> a good one).
>>
> #2 was part of a code review chain.
>
>
>> My comments in #1 weren't agreement, and I didn't see anything else in the
>> thread that would constitute agreement. It appeared at first that you
>> didn't interpret it as agreement either, as you did exactly the right
>> thing
>> and started another code review. However, there were (again) comments
>> outstanding at the time you committed. While there may be early
>> indications
>> of a path toward a submittable form for the patch, or general support for
>> the direction of a patch, these aren't the same as having the comments
>> addressed and an explicit approval to conclude the review.
>>
> The reason I started a complete different chain was the original review
> chain became too long, and thought this review request could get lost in
> the whole thread.
>
>
>> I think you need to take a step back and slow down a bit. I know you may
>> be
>> under pressure to implement this quickly, but that isn't a justification
>> for ignoring or responding poorly to feedback from the community. You've
>> recently caused folks in the project to spend a significant amount of
>> their
>> time trying to explain things, unbreak things, etc. That isn't fair to
>> them
>> or the project as a whole, and it uses up the time that Rui and others
>> could spend reviewing your patch. I don't want to see this continue, as it
>> isn't healthy, and is delaying ELF support in LLD.
>>
> This patch didnt break anything.  Essentially did what was discussed at
> #2. This has slowed me down a lot :)
>
> I am still waiting for review comments that I missed handling:(


Apparently the thing that's missed in the code review was the unused field
_file. You removed it after submitting the code, fine, but that was a open
question when you submitted without LGTM. There was a design discussion
regarding FileToMutable class (I still don't like it) was going on, as you
once thought my suggestion would be better than FileToMutable, so there
really was an open question.

And one thing is that I mentioned about the unused field three times in the
code review. The last six emails between you and me were to repeat the same
thing about the dead field and suggesting a better design. You've lost your
time by rushing to commit it. Please take your time to read my comments and
your code. That would actually make the process faster.

 Instead, I would encourage you to take specific care with each code review,
>> work to address all of the comments, and wait patiently for review to
>> complete. That is not only the best way forward and the one required in an
>> open source project like LLVM, but it will actually be the fastest as
>> well.
>>
> Sure.
>
> Thanks
>
> Shankar Easwaran
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by the Linux Foundation
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131029/4fbda9fd/attachment.html>