r213171 - Make clang's rewrite engine a core feature

[Douglas Gregor]

> On Jul 17, 2014, at 4:24 AM, Alp Toker <alp at nuanti.com> wrote:
> 
> 
> On 17/07/2014 09:21, Richard Smith wrote:
>> On Wed, Jul 16, 2014 at 8:07 PM, Alp Toker <alp at nuanti.com <mailto:alp at nuanti.com>> wrote:
>> 
>>         My reply here was me taking my earliest opportunity to engage
>>        and discuss. =)
>> 
>> 
>>    I don't understand your sudden expectation to be copied into the
>>    discussion here. It's a module you haven't committed to at all
>>    recently or as far back as I can remember. I've been relatively
>>    active in this corner of the codebase and the change was discussed
>>    and approved on the commits list.
>> 
>> 
>> There were two changes here. One (the file reorganization) was not discussed and not approved.
> 
> It's normal to reformat code, reorganise files, fix up style in areas you're working on. This change had no functional impact at all and is purely a cleanup, and you said yourself it looks to be an improvement.

Yes, but it’s a matter of degrees. Reforming some code, splitting one .cpp file into multiple .cpp files, renaming a file whose purpose has shifted… these are all normal cleanup activities. Moving and renaming a library with more than 10 source files, and which has been in Clang for a very long time and has external clients, is a more significant change bordering on architectural. It’s reasonable to request/expect some discussion before committing the latter.

> 
>> For the other, only two hours passed between the first suggestion of the change and the commit,
> 
> Two hours is a comparatively long window.
> 
> People at your organisation frequently commit LLVM and clang patches within 5-15 minutes of a colleague posting the patch:
> 
> Every day commits get pushed to SVN before the mailing list has even *forwarded* the original patch, so I find a complaint about a lengthy two hour window hard to grasp. This is totally not going to change.

Nor should it, but we do discuss more-significant changes first, and two hours is too short for more-significant changes.

>> the discussion happened on an unrelated patch review thread,
> 
> The other thread was entirely related to this change. It also hit the same keywords about "moving files" and "rewrite" and enabling features so the thread had complete topical overlap, and an equivalent audience. It would have been most unconventional to split the thread.

One could very easily have skipped the previous thread as “not interesting” because its subject did not imply as large a change as it ended up implying. It’s reasonable to ask that changes on this scale get their own threads with a subject that implies the scale of the change.

>> The level and duration of preceding discussion was not sufficient for either of these changes.
> 
> If you want to introduce a mandatory review window, please propose that as an amendment to the developer policy on llvmdev. There's nothing actionable I can change based on this discussion and I need to make it clear that I believe we've followed best practice here. The mailing list workflow is central to the project and that needs to be said loud and clear.


Obviously, mandatory review windows don’t make sense, and haven’t actually been proposed, so let’s ignore that bit. 

Fundamentally, we agree that small things should be post-commit—reviewed and that larger things should have some discussion first. There is no hard-and-fast rule we can come up with to separate “small” from “larger” things. In a fast-moving community, that means we’ll see disagreement from time to time. The advice here is that renaming libraries and moving a dozen source files is considered “larger” and needs some mention on the mailing lists, with its own separate thread, that gives others a day or so to respond. 

	- Doug