<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 16, 2016 at 11:36 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> It may be fine to emit a warning, but there's no need to abort unless<br>
> the user wanted to do something that can't (or is unreasonable to) be<br>
> done. Other tests caught actual uses of invalid data.<br>
<br>
</span>Correct. What I mean is that if that test no longer hits a particular<br>
error, you should not just delete the error checking like the patch<br>
did:<br>
<br>
     if (I == LoadSegments.begin())<br>
-      report_fatal_error("Virtual address is not in any segment");<br>
+      return nullptr;<br>
<br>
You should instead add a test where the LoadSegments are needed and<br>
you can't get them because the file is corrupted. If in no case that<br>
information is needed, this is now dead code and a lot more should be<br>
deleted.<br>
<span class=""><br>
>> There is. You deleted error checking code without review.<br>
>><br>
>> Cheers,<br>
>> Rafael<br>
><br>
> That was not an acceptable reason to revert this patch, and there is<br>
> zero precedent for doing this. You just decided that you were right<br>
> and reverted a patch without discussion, and went against the llvm<br>
> developer policy. Do not do this in the future.<br>
<br>
</span>I see the other way. You deleted error checking, introduced code that<br>
didn't conform to the style guide and the patch was not reviewed.</blockquote><div><br></div><div>I think that post-commit review is fine for changes like this (especially since Michael is the code owner for libObject and related tools). Reverting without waiting for Michael to address your post-commit review comments is definitely not in line with typical LLVM development practices (unless this broke a bot, but I don't think it did).</div><div><br></div><div>(keep in mind that Monday was a holiday in the US)</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I<br>
reverted a patch that should not have been committed in the first<br>
place.<br>
<br>
Having reverted it I took the trouble of doing what you should have<br>
done before even sending it for review: I have split the patch and<br>
committed the independent cleanup changes and the follow up feature<br>
which didn't depend on the complex change.<br>
<br>
Cheers,<br>
Rafael<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>