r213171 - Make clang's rewrite engine a core feature

[Richard Smith]

On Wed, Jul 16, 2014 at 1:40 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 16/07/2014 22:00, Richard Smith wrote:
>
>  On Wed, Jul 16, 2014 at 9:48 AM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>     Author: alp
>>     Date: Wed Jul 16 11:48:33 2014
>>     New Revision: 213171
>>
>>     URL: http://llvm.org/viewvc/llvm-project?rev=213171&view=rev
>>     Log:
>>     Make clang's rewrite engine a core feature
>>
>>     The rewrite facility's footprint is small so it's not worth going
>>     to these
>>     lengths to support disabling at configure time, particularly since
>>     key compiler
>>     features now depend on it.
>>
>>     Meanwhile the Objective-C rewriters have been moved under the
>>     ENABLE_CLANG_ARCMT umbrella for now as they're comparatively heavy
>>     and still
>>     potentially worth excluding from lightweight builds.
>>
>>     Tests are now passing with any combination of feature flags. The flags
>>     historically haven't been tested by LLVM's build servers so caveat
>>     emptor.
>>
>>
>> Hi Alp,
>>
>> A reorganization of this magnitude should be discussed before being
>> committed.
>>
>
>
> We've discussed this on the list (http://lists.cs.uiuc.edu/
> pipermail/cfe-commits/Week-of-Mon-20140714/110252.html).


That was a discussion of the removal of the flag, not of the code
reorganization, as far as I can see.

 More generally, changes to the file organization should generally get the
>> nod of approval from the relevant code owner. (That said, I think this
>> reorg is an improvement, so I'm not objecting to the change, just to the
>> process.)
>>
>
>
> I'm sorry if there wasn't much of a window but there was agreement on-list
> that this is the right direction, driven by the need to fix tests which
> have been failing for some time in the run up to 3.5.
>
> There is no code owner either for the feature flag or the rewrite
> infrastructure, and we're doing our best to keep things running.
>

Doug is the code owner for all parts of Clang that don't have another
owner. There is no "keep things running" going on here; moving the files
doesn't even provide much value, it's mostly just pure churn.

I feel strongly that best practice has been followed here including the
> relevant pre-commit discussion on the public mailing list which is why I'm
> concerned here.
>
> Richard, can you explain why you're objecting to the process so we can
> address that?


As far as I can see, you didn't ask for comments before moving 15 files to
a different place (and moving them to a different clang library in the
process). This sort of reorg change has the potential to disrupt people in
various ways (if they have pending changes to those files, or if they have
an external build system, or if they link against clang libraries, for
example), so it should be mentioned somewhere visible. (For instance, you
broke the LLDB build with this change.)

[You also said "I was just about to make a commit making the rewriter
mandatory actually." which makes me think that maybe you wouldn't have
mentioned anything prior to commit if Takumi hadn't committed his change,
but maybe I'm reading too much into that.]

 It also would have been courteous to ask if anyone was using the
>> CLANG_ENABLE_REWRITER flag before removing it. See r170135, which gives a
>> justification for including this flag, which doesn't seem to have gone away.
>>
>
> We continue to support the original use case for lightweight builds
> following this change. The core rewrite machinery is only ~2000 LoC
> compared to the static analyzer at > 38,000 LoC, and ARCMT at > 8,000 LoC.
> I'm sure Roman would agree with this commit though he's not working on the
> frontend these days.
>

Again, I'm asking about process, not about the specifics of the change. A
post to a post-commit review thread for a semi-unrelated change isn't
really sufficiently visible for removing a user-facing configuration option
(it's not reasonable to expect everyone to read all commit mail). A mail to
cfe-dev (or at least creating a new thread on cfe-commits) would be better
for this sort of thing in future.

 Thanks!
>>
>
> PS. If you're interested in reorg proposals, I have a couple more posted
> to the list awaiting response.


If you point me at them I should be able to give you a quick thumbs-up.
Thanks again!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140716/b92235c2/attachment.html>