r213171 - Make clang's rewrite engine a core feature

[Alp Toker]

On 17/07/2014 01:07, Richard Smith wrote:
> On Wed, Jul 16, 2014 at 1:40 PM, Alp Toker <alp at nuanti.com 
> <mailto: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> <mailto: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.

I don't remember a time we've run simple changes like this by Doug.

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

No, the files haven't been moved to a different library.

One internal library was renamed because the "Core" suffix is 
meaningless and not used anywhere else in the frontend. This is routine.


> This sort of reorg change has the potential to disrupt people in 
> various ways (if they have pending changes to those files,

Any modern version control system will track file moves without 
prompting the user. This isn't a good reason to avoid file moves at all.


> or if they have an external build system

What's the real motivation here? Did my commit break the the Android 
build system?

I don't think it's reasonable to expect the community to support yet 
more out-of-tree build systems. We already spend enough time keeping the 
two in-tree synchronised.

The best we can do here, as with any code change, is to describe updates 
in the commit log that help external integrators adapt.


> , or if they link against clang libraries, for example), so it should 
> be mentioned somewhere visible.

We've never documented this library for consumers of the internal API, 
and it's certainly not part of the stable public API. Why mention a 
change beyond the commit itself?

Users of the internal C++ API agree to track upstream changes. Much of 
my work in the last year has related to making the C++ API easier to 
embed so I have a sense of the impact here.

We're all on the same boat, and I've had to adapt our own internal 
clang-based project of following this commit, same as happens with other 
routine commits every day.

> (For instance, you broke the LLDB build with this change.)

Zach has been brilliant and fixed the lldb build in r213181 with a 
one-line tweak.

I commit similar build fixes every other day. What at all does that have 
to do with process?

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

We'll never know whether I might have pinged the list, committed today 
or perhaps left the change until after the 3.5 release as I actually 
said in the other mail.


>         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

Takumi's commit was directly related to this change, and there were 
back-and-forth mails with opportunities to engage and discuss.

Granted the discussion wasn't copied to cfe-dev/cfe-users, but I think 
we already had a very reasonable amount of process for a trivial commit 
like this with minimal functional change.

> (it's not reasonable to expect everyone to read all commit mail).

Disagree. Reading commit mail is the *only* way to track internal 
internal API updates.

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

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140707/109896.html 
is a recent one.


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