<div class="gmail_quote">On Fri, Jan 27, 2012 at 4:21 PM, Joerg Sonnenberger <span dir="ltr"><<a href="mailto:joerg@britannica.bec.de">joerg@britannica.bec.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Fri, Jan 27, 2012 at 03:16:17PM -0800, Douglas Gregor wrote:<br>
> > Reverting just this change is inacceptable. It fixes a major regression<br>
> > for cross-compiling support from your earlier changes, without<br>
> > introducing a bag of new ones.<br>
><br>
> There is a legitimate concern that your change is not the right<br>
> direction. Please revert, and we can discuss how best to implement<br>
> this functionality.<br>
<br>
</div>That doesn't answer the question of how to deal with the original change<br>
that broke it. So before randonly reverting changes -- where should the<br>
user triple belong?<br></blockquote><div><br></div><div>Reviewers and the code owner for Clang have already looked at this patch, and provided feedback that we need a different approach. The accepted path forward is to revert the patch, produce a new one addressing that feedback and discuss it on the mailing list.</div>
<div><br></div><div>In this case, the patch never got discussed at all before committing, and so it would seem like a good place for you to start would be to revert and submit exactly this patch to the mailing list for review where we can work through any problems with it.</div>
<div> </div><div><br></div><div>In any event, I've reverted this patch according to Doug's request in rNNNNN.</div><div><br></div><div>I'm working on an alternate design since I am interested in the use case, and we still don't have a proposed patch for proper code review where design discussions can take place, but I'll respond here at a high level to these items:</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(a) Driver. Old location before the driver refactoring with FIXME as it<br>
required parsing the arguments.<br></blockquote><div><br></div><div>It was also never intended ta capture the *user*'s triple, or anything other than the *default* triple. Relying on that was relying on an accident of the code, and one which nothing else used or enforced.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(b) Toolchain. That's what implemented in this change. This also allows<br>
platforms to deal with more limited toolchains, e.g. old as doesn't<br>
support --32 if built only for i386.<br></blockquote><div><br></div><div>This is where the functionality belongs, but your patch doesn't implement it there in a clean manner. I've outlined some of the issues in the revert commit log, but to repeat them here:</div>
<div><br></div><div>1) It incorrectly prefers the "user" triple rather than trying to find an effective *toolchain* triple. This is really important if users ask for i686--linux-gnu, but we find an x86_64--linux-gnu toolchain that happens to support multiarch operation through the '-m32' flag.</div>
<div><br></div><div>2) It forces every toolchain interface to carry the burden of tracking both triples, when instead the base classes can handle all of this logic.</div><div><br></div><div>3) It uses very confusing naming in the Driver change, with "Target" and "TargetTriple" both naming triples, but not the same triple, and one of them not the target triple.</div>
<div><br></div><div>4) It needlessly includes both triples in the cache key of the ToolChain.</div><div><br></div><div><br></div><div>Anyways, we should continue this discussion on proposed patches rather than on the commit log for this one. I hope to have the first patch in the series out quite soon.</div>
</div>