<div dir="ltr"><div class="gmail_extra">Rui has already reverted this patch, but I wanted to provide some very clear feedback at an LLVM project level to this entire thing.</div><div class="gmail_extra"><br><div class="gmail_quote">
On Thu, Oct 24, 2013 at 8:31 AM, Shankar Easwaran <span dir="ltr"><<a href="mailto:shankare@codeaurora.org" target="_blank" class="cremed">shankare@codeaurora.org</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 10/24/2013 12:23 AM, Rui Ueyama wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I did not expect you were going to submit your change with disabling so<br>
many tests. It disabled even pretty basic tests for COFF. This is really<br>
bad. Please don't do that.<br>
</blockquote></div>
The problem was not in the patch as what I see. The problem is because the relocation types have not been added by pecoff.<br>
<br>
ErrorOr<Reference::Kind><br>
PECOFFLinkingContext::<u></u>relocKindFromString(StringRef str) const {<br>
  return make_error_code(<u></u>YamlReaderErro<span class="il">r</span>::illegal_<u></u>value);<br>
}<br>
<br>
ErrorOr<std::string><br>
PECOFFLinkingContext::<u></u>stringFromRelocKind(Reference:<u></u>:Kind kind) const {<br>
  return make_error_code(<u></u>YamlReaderErro<span class="il">r</span>::illegal_<u></u>value);<br>
}<br>
<br>
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.<br>
<br>
*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).</blockquote>
</div><br>This is not acceptable in LLVM. Specifically:</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">-Chandler</div></div>