[lld] r193300 - [PassManager] add ReaderWriter{Native, YAML} to the Driver.

[Chandler Carruth]

Rui has already reverted this patch, but I wanted to provide some very
clear feedback at an LLVM project level to this entire thing.

On Thu, Oct 24, 2013 at 8:31 AM, Shankar Easwaran
<shankare at codeaurora.org>wrote:

> On 10/24/2013 12:23 AM, Rui Ueyama wrote:
>
>> I did not expect you were going to submit your change with disabling so
>> many tests. It disabled even pretty basic tests for COFF. This is really
>> bad. Please don't do that.
>>
> The problem was not in the patch as what I see. The problem is because the
> relocation types have not been added by pecoff.
>
> ErrorOr<Reference::Kind>
> PECOFFLinkingContext::**relocKindFromString(StringRef str) const {
>   return make_error_code(**YamlReaderError::illegal_**value);
> }
>
> ErrorOr<std::string>
> PECOFFLinkingContext::**stringFromRelocKind(Reference:**:Kind kind) const
> {
>   return make_error_code(**YamlReaderError::illegal_**value);
> }
>
> These two are empty, this has to be fixed. This was the reason behind the
> failures for PECOFF. There are some more inherent assumptions that are
> being used which the new RoundTrip{YAML, Native} changes uncovered.
>
> *I am going to fix the ELF cases*, but I am going to leave it to the
> flavor owners for changes to their respective flavors as I dont have much
> knowledge on what kind of factors to be considered ( as these might be some
> assumptions that could break).


This is not acceptable in LLVM. Specifically:

1) You asked for pre-commit review, there were clearly unanswered questions
so that seems appropriate. You then submitted 4 hours after posting an
update without any LGTM or other indication that others were happy with
your changes. That is unacceptable. If you think it needs pre-commit review
(and this did), you need to see that review through.

2) You left the tree in an unusable state by introducing a change that
regressed essentially every user of LLD. That is unacceptable. The tree
should always be in a "shippable" state. You need to fix the bugs your
patch exposes prior to submission when they are this prevalent.

3) You seem to think that you don't have to update parts of LLD other than
ELF, but that isn't true. While you can certainly ask the other maintainers
for help, if this is a change you want to go in, you are the person
ultimately responsible for ensuring the other flavors are ready.

In case you think that #3 isn't really an issue, look at the development of
LLVM's backends. I assure you that Jakub and Andy don't know about or care
about MSP430 or Mips, but when they go to change the backend, they update
those targets to support it.

The only cases where you can reasonably break a port would be if you have
worked hard to contact the maintainers of it, explain why it was relying on
poorly founded assumptions that need to change and why you can't make an
appropriate change due to the significance / lack of knowledge of their
port, and fail to get any support or help from those maintainers.
Typically, this is followed swiftly by the *removal* of that port because
we can't make progress.

But here you have active maintainers responding promptly to code review, so
I think that the burden does in fact fall upon you to either make the
necessary changes, or incorporate suggested changes provided by Rui and
Nick.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131024/130e2669/attachment.html>