On Wed, Jul 16, 2014 at 4:03 PM, Alp Toker <a...@nuanti.com> wrote:
>
> On 17/07/2014 01:07, Richard Smith wrote:
>
>> On Wed, Jul 16, 2014 at 1:40 PM, Alp Toker <a...@nuanti.com <mailto:
>> a...@nuanti.com>> wrote:
>>
>>
>> On 16/07/2014 22:00, Richard Smith wrote:
>>
>> On Wed, Jul 16, 2014 at 9:48 AM, Alp Toker <a...@nuanti.com
>> <mailto:a...@nuanti.com> <mailto:a...@nuanti.com
>>
>> <mailto:a...@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.
I was contesting your claim that there is no code owner here. (And actually
I completely agree with you that asking for code owner approval is too
strong here, but asking for some amount of review seems prudent.)
> 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.
I was referring to internal libraries; these files have moved to a
different internal library. But the specifics of the change aren't really
relevant to the point.
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?
>
The real motivation is to ensure that changes like this get discussed
before they land. That's all. I don't really care about breaking the build
of external projects that are depending on our implementation details (and
no, I don't know of any that you broke other than LLDB, and I'm not sure
why you'd ask about Android) because a certain amount of churn is a
valuable argument to persuade people to upstream important changes. But
it's not OK to make broad changes unilaterally and without discussion.
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.
We can do better: we can discuss complex or non-obvious changes prior to
commit, as the developer policy requires.
, 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?
>
Again, I'm asking for discussion prior to commit. I don't think that's
unreasonable.
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.
>
You first mentioned this at ~8am PDT (the majority timezone of Clang
developers) and committed less than 2 hours later. My reply here was me
taking my earliest opportunity 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.
Removing a build configuration option would usually have much more process
than this.
(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.
I don't think it's reasonable to expect everyone to read all review
comments on a commit that is not of interest to them. Starting a new thread
would have helped.
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.
Thanks, I've replied on that thread.
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
