<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 16, 2014 at 1:40 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
On 16/07/2014 22:00, Richard Smith wrote:<div><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
On Wed, Jul 16, 2014 at 9:48 AM, 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>
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 worth going<br>
to these<br>
lengths to support disabling at configure time, 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 comparatively heavy<br>
and still<br>
potentially worth excluding from lightweight builds.<br>
<br>
Tests are now passing with any combination of feature flags. The flags<br>
historically haven't been tested by LLVM's build servers so caveat<br>
emptor.<br>
<br>
<br>
Hi Alp,<br>
<br>
A reorganization of this magnitude should be discussed before being committed.<br>
</blockquote>
<br>
<br></div>
We've discussed this on the list (<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>).</blockquote>
<div><br></div><div>That was a discussion of the removal of the flag, not of the code reorganization, as far as I can see.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.)<br>
</blockquote>
<br>
<br></div>
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.<br>
<br>
There is no code owner either for the feature flag or the rewrite infrastructure, and we're doing our best to keep things running.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.<br>
<br>
Richard, can you explain why you're objecting to the process so we can address that?</blockquote><div><br></div><div>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.)</div>
<div><br></div><div>[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.]</div>
</div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.<br>
</blockquote>
<br></div>
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.<br>
</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Thanks!<br>
</blockquote>
<br>
PS. If you're interested in reorg proposals, I have a couple more posted to the list awaiting response.</blockquote><div><br></div><div>If you point me at them I should be able to give you a quick thumbs-up. Thanks again!</div>
</div></div></div>