[cfe-commits] [PATCH] Add PNaCl ABIInfo

[Douglas Gregor]

On Sep 23, 2011, at 1:23 AM, David Meyer wrote:
>> In short, several of us are objecting to your choice of ABI, which we do
>> have the right to do, since that ABI is intrinsically intended to be a
>> permanent constraint on LLVM and Clang.
> 
> I have not seen an objection from anybody else besides you, and Eli
> has already approved this change. What is the procedure for resolving
> this kind of dispute?

The code owners are ultimately responsible for making decisions on code they own:

	http://llvm.org/docs/DeveloperPolicy.html#owners

>>> While our ABI is not 100% finalized, until this patch is committed, we
>>> can't do any external testing with Clang. (unless we create our own
>>> branch, which we'd rather not)
>> 
>> I'm sorry, but we do expect code to go through code review, which
>> includes a thorough review of the underlying motivations and technical
>> design.
> 
> This doesn't make sense. Ultimately, the PNaCl project controls its
> own ABI. And I don't mean me. I was not involved in the decision to
> use first-class struct arguments, and if that decision needs to be
> altered, I'm not the person who has the final say on it.

Your patch depends on that decision, and that decision may be problematic for the completion of your overall task, so it's clearly relevant.

> Anybody on the Clang team is welcome to offer advice for PNaCl, but
> you must first appreciate the strange technical requirements we have
> to satisfy, and that requires learning about the NaCl environment and
> how we fit into the rest of the NaCl project.
> 
> And I'm sorry we can't guarantee this code won't change. I had no idea
> the Clang codebase was so.. immaculate? Our project is a work in
> progress. That means we sometimes check in code which is not
> feature-complete, and possibly even contains bugs.


Work-in-progress is fine, so long as it's clear that you're on a path to a proper solution. The concern about the use of first class aggregates highlights that it is *not* clear that you can get to a proper solution by starting with your patch. That is an absolutely legitimate concern and a good reason not to accept this patch now, regardless of the size of the patch.

Generalizing further: we like incremental development, but we don't like thrashing. We want Clang to grow to support different architectures and environments, but not at the cost of Clang's design or internal consistency. A poorly-implemented feature is worse than no feature at all, because it places a long-term maintenance burden on us as a community and makes it harder to implement that feature properly later on. 

	- Doug