[PATCH] fix llvm linker issue with dbg.declare

[Bob Wilson]

On Jan 30, 2013, at 3:03 PM, Nick Lewycky <nicholas at mxc.ca> wrote:

> Bob Wilson wrote:
>> Eli,
>> 
>> This is not a new patch review policy. It is the old policy:
>> 
>> LLVM has a code review policy. Code review is one way to increase the
>> quality of software. We generally follow these policies:
>> 
>> 1. All developers are required to have significant changes reviewed
>>    before they are committed to the repository.
>> 2. Code reviews are conducted by email, usually on the llvm-commits list.
>> 3. Code can be reviewed either before it is committed or after. We
>>    expect major changes to be reviewed before being committed, but
>>    smaller changes (or changes where the developer owns the component)
>>    can be reviewed after commit.
>> 4. The developer responsible for a code change is also responsible for
>>    making all necessary review-related changes.
>> 5. Code review can be an iterative process, which continues until the
>>    patch is ready to be committed.
>> 
>> I specifically asked Manman to commit this without waiting for a review,
>> because I do not consider it to be a "significant change".
> 
> I want to make a quick point here, as there's something unclear about this line which concerns me.
> 
> The right way to convince someone to commit a patch is by reviewing it. Even when you are not an expert in the area, the fact both of you have looked at it can raise your confidence in a patch up to the "good enough for post-commit review" bar.
> 
> But, please don't ever let me hear that someone has committed a patch they still think needs pre-commit review, or otherwise shouldn't be committed, just because their manager has asked them to. That wouldn't be okay.
> 
> Nick

Yes, I agree, Nick.  It would have been better for me to reply to the patch and give it an approval.  But, I am concerned that we seem to be moving away from our usual policy of review-after-commit, even for simple bug-fix patches like this.  My comment to Manman was not "ignore the review process" but rather "you don't need a pre-commit review for that".

> 
> It was nice
>> of her to ask for a pre-commit review, but I did not think that was
>> necessary. There have been a lot of new contributors to LLVM recently,
>> and we have consequently been asking for more pre-commit reviews, but in
>> general, experienced LLVM developers can go with post-commit reviews for
>> changes like this. Manman has been actively contributing to LLVM for
>> almost a year, so I think we can trust her.
> 
>> 
>> On Jan 30, 2013, at 10:52 AM, Chad Rosier <mcrosier at apple.com
>> <mailto:mcrosier at apple.com>> wrote:
>> 
>>> 
>>> On Jan 30, 2013, at 10:37 AM, Eli Bendersky <eliben at google.com
>>> <mailto:eliben at google.com>> wrote:
>>> 
>>>>>> Since I got no response on this patch and we need this patch soon,
>>>>>> I committed at r173946.
>>>>> 
>>>>> Manman,
>>>>> It may have been best to continue pressing for a review first.
>>>>> 
>>>> 
>>>> Yes.
>>>> 
>>>>>>> If you have any concern about this patch, please review after commit.
>>>>>>> 
>>>>>> 
>>>>>> Ah, it's good to know there is a new patch review policy for LLVM.
>>>>>> Thanks, Manman.
>>>>> 
>>>>> Eli,
>>>>> What was the point of this comment?
>>>> 
>>>> The point was to express the same thing you said above. The sarcastic
>>>> tone may have been uncalled for, but stems from bitter experience.
>>> 
>>> I'm sorry you've had a number of poor experiences, but lets try to be
>>> constructive/positive despite them.
>>> 
>>>> Having had the pleasure (on multiple occasions for multiple code
>>>> areas) to chase code owners for weeks with multiple list & IRC pings
>>>> before getting the LGTM, it hurts to see the honor code broken by a
>>>> review-less commit made less than 48 hours after the initial request,
>>>> because "we need this patch soon".
>>> 
>>> Thanks for the clarification, Eli. I'm sure it's very frustrating.
>>> 
>>>> Eli
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>