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

[Michael Spencer via llvm-commits]

On Tue, Feb 16, 2016 at 11:36 AM, Rafael Espíndola
<rafael.espindola at gmail.com> 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.

Ah, I thought I had modified a test to expose this, but apparently not.

>
>>> 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,

I added a feature with tests that were missing some cases. I should
have had the test. It does not require reverting the patch to fix
this.

> introduced code that didn't conform to the style guide

Where? dynamic_symbols()? I was following the style guide here:

"If you are extending, enhancing, or bug fixing already implemented
code, use the style that is already being used so that the source is
uniform and easy to follow."

Code in this file and other places in llvm that have replaced begin,
end pairs with a range have used this style.

> and the patch was not reviewed.

I do not need pre-commit review to make changes to readobj.

> I
> reverted a patch that should not have been committed in the first
> place.

Per developer policy: Commits that violate these quality standards
(e.g. are very broken) may be reverted. This is necessary when the
change blocks other developers from making progress.

Reverting is reserved for very broken commits that block progress.
This was not a very broken commit, and it did not block progress. What
has blocked progress is you reverting the commit and having to argue
with you about it.

- Michael Spencer

>
> 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