<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 16, 2014 at 4:03 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 17/07/2014 01:07, Richard Smith wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
On Wed, Jul 16, 2014 at 1:40 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
<br>
On 16/07/2014 22:00, Richard Smith wrote:<br>
<br>
On Wed, Jul 16, 2014 at 9:48 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div>
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><div><div><br>
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>>> wrote:<br>
<br>
Author: alp<br>
Date: Wed Jul 16 11:48:33 2014<br>
New Revision: 213171<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=213171&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=213171&view=rev</a><br>
Log:<br>
Make clang's rewrite engine a core feature<br>
<br>
The rewrite facility's footprint is small so it's not<br>
worth going<br>
to these<br>
lengths to support disabling at configure time,<br>
particularly since<br>
key compiler<br>
features now depend on it.<br>
<br>
Meanwhile the Objective-C rewriters have been moved under the<br>
ENABLE_CLANG_ARCMT umbrella for now as they're<br>
comparatively heavy<br>
and still<br>
potentially worth excluding from lightweight builds.<br>
<br>
Tests are now passing with any combination of feature<br>
flags. The flags<br>
historically haven't been tested by LLVM's build servers<br>
so caveat<br>
emptor.<br>
<br>
<br>
Hi Alp,<br>
<br>
A reorganization of this magnitude should be discussed before<br>
being committed.<br>
<br>
<br>
<br>
We've discussed this on the list<br>
(<a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140714/110252.html" target="_blank">http://lists.cs.uiuc.edu/<u></u>pipermail/cfe-commits/Week-of-<u></u>Mon-20140714/110252.html</a>).<br>
<br>
<br>
That was a discussion of the removal of the flag, not of the code reorganization, as far as I can see.<br>
<br>
More generally, changes to the file organization should<br>
generally get the nod of approval from the relevant code<br>
owner. (That said, I think this reorg is an improvement, so<br>
I'm not objecting to the change, just to the process.)<br>
<br>
<br>
<br>
I'm sorry if there wasn't much of a window but there was agreement<br>
on-list that this is the right direction, driven by the need to<br>
fix tests which have been failing for some time in the run up to 3.5.<br>
<br>
There is no code owner either for the feature flag or the rewrite<br>
infrastructure, and we're doing our best to keep things running.<br>
<br>
<br>
Doug is the code owner for all parts of Clang that don't have another owner.<br>
</div></div></blockquote>
<br>
I don't remember a time we've run simple changes like this by Doug.</blockquote><div><br></div><div>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.)<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
There is no "keep things running" going on here; moving the files doesn't even provide much value, it's mostly just pure churn.<br>
<br>
I feel strongly that best practice has been followed here<br>
including the relevant pre-commit discussion on the public mailing<br>
list which is why I'm concerned here.<br>
<br>
Richard, can you explain why you're objecting to the process so we<br>
can address that?<br>
<br>
<br>
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).<br>
</blockquote>
<br></div>
No, the files haven't been moved to a different library.<br>
<br>
One internal library was renamed because the "Core" suffix is meaningless and not used anywhere else in the frontend. This is routine.</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This sort of reorg change has the potential to disrupt people in various ways (if they have pending changes to those files,<br>
</blockquote>
<br></div>
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.<div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
or if they have an external build system<br>
</blockquote>
<br></div>
What's the real motivation here? Did my commit break the the Android build system?<br></blockquote><div><br></div><div>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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
The best we can do here, as with any code change, is to describe updates in the commit log that help external integrators adapt.</blockquote><div><br></div><div>We can do better: we can discuss complex or non-obvious changes prior to commit, as the developer policy requires.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
, or if they link against clang libraries, for example), so it should be mentioned somewhere visible.<br>
</blockquote>
<br></div>
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?<br></blockquote><div><br></div><div>Again, I'm asking for discussion prior to commit. I don't think that's unreasonable.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It also would have been courteous to ask if anyone was using<br></blockquote></div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
the CLANG_ENABLE_REWRITER flag before removing it. See<br>
r170135, which gives a justification for including this flag,<br>
which doesn't seem to have gone away.<br>
<br>
<br>
We continue to support the original use case for lightweight<br>
builds following this change. The core rewrite machinery is only<br>
~2000 LoC compared to the static analyzer at > 38,000 LoC, and<br>
ARCMT at > 8,000 LoC. I'm sure Roman would agree with this commit<br>
though he's not working on the frontend these days.<br>
<br>
<br>
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<br>
</blockquote>
<br></div>
Takumi's commit was directly related to this change, and there were back-and-forth mails with opportunities to engage and discuss.<br></blockquote><div><br></div><div>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. =)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote><div><br></div><div>
Removing a build configuration option would usually have much more process than this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(it's not reasonable to expect everyone to read all commit mail).<br>
</blockquote>
<br></div>
Disagree. Reading commit mail is the *only* way to track internal internal API updates.</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
Thanks!<br>
<br>
<br>
PS. If you're interested in reorg proposals, I have a couple more<br>
posted to the list awaiting response.<br>
<br>
<br>
If you point me at them I should be able to give you a quick thumbs-up. Thanks again!<br>
</blockquote>
<br>
</div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140707/109896.html" target="_blank">http://lists.cs.uiuc.edu/<u></u>pipermail/cfe-commits/Week-of-<u></u>Mon-20140707/109896.html</a> is a recent one.</blockquote>
<div><br></div><div>Thanks, I've replied on that thread.</div></div></div></div>