r213171 - Make clang's rewrite engine a core feature

[Alp Toker]

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.

As second most-most active developer in the clang rewriter module in the 
last year I can *certainly* make a change like this following some 
back-and-forth on the list and you are welcome to provide post-commit 
review.

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

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

> and it removed a supported configuration option.

The configuration option was not supported. We actually use these flags 
and that's why we're maintaining them -- so we have a good idea what is 
supported and what isn't.

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

-- 
http://www.nuanti.com
the browser experts