[llvm] r260488 - [readobj] Handle ELF files with no section table or with no program headers.

[Rafael Espíndola via llvm-commits]

On 16 February 2016 at 15:22, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Tue, Feb 16, 2016 at 11:36 AM, Rafael Espíndola
> <llvm-commits at lists.llvm.org> wrote:
>>
>> > It may be fine to emit a warning, but there's no need to abort unless
>> > the user wanted to do something that can't (or is unreasonable to) be
>> > done. Other tests caught actual uses of invalid data.
>>
>> Correct. What I mean is that if that test no longer hits a particular
>> error, you should not just delete the error checking like the patch
>> did:
>>
>>      if (I == LoadSegments.begin())
>> -      report_fatal_error("Virtual address is not in any segment");
>> +      return nullptr;
>>
>> You should instead add a test where the LoadSegments are needed and
>> you can't get them because the file is corrupted. If in no case that
>> information is needed, this is now dead code and a lot more should be
>> deleted.
>>
>> >> There is. You deleted error checking code without review.
>> >>
>> >> Cheers,
>> >> Rafael
>> >
>> > That was not an acceptable reason to revert this patch, and there is
>> > zero precedent for doing this. You just decided that you were right
>> > and reverted a patch without discussion, and went against the llvm
>> > developer policy. Do not do this in the future.
>>
>> I see the other way. You deleted error checking, introduced code that
>> didn't conform to the style guide and the patch was not reviewed.
>
>
> 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).

It didn't break a bot because he deleted the error handling.

> (keep in mind that Monday was a holiday in the US)

It was one in Ontario too.
Also keep in mind that I did the work of splitting the patch and
putting it back in. Without a revert it was really hard to be sure we
were getting back the error handling the patch deleted and making sure
all the parts were reviewed.

Cheers,
Rafael