r213171 - Make clang's rewrite engine a core feature

[Sean Silva]

On Thu, Jul 17, 2014 at 3:43 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 18/07/2014 00:13, Douglas Gregor wrote:
>
>> 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.
>>
>
> Sure, but let's hold the rest of the community to those standards too?
>
> Right now there's a lot of commit activity that's not going through the
> list at all and which needs at least as much scrutiny as a reorg commit
> that actually had a two hour window on the list.


If what you're saying is correct then let's discuss in a focused -dev
thread.

-- Sean Silva


>
>
>
>
>  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
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/0629de2b/attachment.html>