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

[Chandler Carruth]

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.


Yes, but you didn't do post-commit review, and you shouldn't have -- this
is a very large patch which has many non-obvious issues (as shown by the
problems on other flavors and the code review comments).

Once you ask for pre-commit review, you really need to follow through.
You've asked one of your fellow contributors to take their time to help
make your patch better.


>  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.


Ok, but it was not part of any of the code review chains germain to the
whole patch. It was discussing a single, isolated technical aspect.


>
>
>> 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.


But that also drops context with the previous review. Please be cautious
about doing this. If you're not sure, ask your reviewer what they would
prefer. Remember that you are asking them to spend their time helping you.


>
>
>> 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 :)
>

It has slowed down *everyone* a lot. A large, complex change with many
interactions on many targets is not going to be a fast patch to commit. =]
I'm sorry that this has slowed you down, but my priority is getting the
development practices into a healthy state. Right now, it's hurting the
entire community.


>
> I am still waiting for review comments that I missed handling:(
>

I am quite certain they are still in the Phabricator code review thread.
I'm sure Rui could ping them.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131029/86b70cec/attachment.html>