On Fri, Jan 27, 2012 at 4:21 PM, Joerg Sonnenberger <[email protected] > wrote:
> On Fri, Jan 27, 2012 at 03:16:17PM -0800, Douglas Gregor wrote: > > > Reverting just this change is inacceptable. It fixes a major regression > > > for cross-compiling support from your earlier changes, without > > > introducing a bag of new ones. > > > > There is a legitimate concern that your change is not the right > > direction. Please revert, and we can discuss how best to implement > > this functionality. > > That doesn't answer the question of how to deal with the original change > that broke it. So before randonly reverting changes -- where should the > user triple belong? > 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. 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. In any event, I've reverted this patch according to Doug's request in rNNNNN. 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: (a) Driver. Old location before the driver refactoring with FIXME as it > required parsing the arguments. > 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. (b) Toolchain. That's what implemented in this change. This also allows > platforms to deal with more limited toolchains, e.g. old as doesn't > support --32 if built only for i386. > 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: 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. 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. 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. 4) It needlessly includes both triples in the cache key of the ToolChain. 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
