r213171 - Make clang's rewrite engine a core feature

[Alp Toker]

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


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

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?

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

> Thanks!

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

Alp.

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