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

[Rafael Espíndola via llvm-commits]

> 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
reverted a patch that should not have been committed in the first
place.

Having reverted it I took the trouble of doing what you should have
done before even sending it for review: I have split the patch and
committed the independent cleanup changes and the follow up feature
which didn't depend on the complex change.

Cheers,
Rafael