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

[Rafael Espíndola via llvm-commits]

In order to try to make this productive I have split out every
improvement, split the remaining features in two patches and replied
with comments on both.

This email hopefully concludes my points on the meta part of the thread.

> I don’t know the details of what is going on here, but I don’t think either of your perspectives is “obviously" right: the ideal is somewhere in between.  The intent of the LLVM development approach is to optimize for the case where patches are uncontentious and “obviously correct”, but to make room for mistakes.
>
> Patch review would be meaningless if no problems/improvements were ever found, and as such, this necessarily means that problems/improvements will be found after the patch goes in.  The developer policies specifically allow for either to problem to be fixed in place, or for the patch to be reverted and a better fix committed on top of it.  Which path is taken is something of a judgement call, which is why I don’t think either of you is “obviously” right.
>
> The essential part of living with this model is to realize that the reversion of a patch is something that *will* happen, and shouldn’t be taken personally.  When I personally drop in patches that break things, or are contentious, I’m generally happy to have someone revert it and sort it out with them later - even if it means that I end up committing exactly the same patch again.

I fully agree. I have had my share of patches that I thought were
"obviously correct" and were at least non-obvious to someone else and
were reverted for further discussion.

> On Feb 16, 2016, at 12:39 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> It *reverted* work that I had done, so you reverted code first. I got
>> it back to what it was. So yes, it was blocking progress. It was
>> blocking making sure we still had the features we had before you
>> decided you could just delete tests.
>
> I see what you’re saying Rafael, but this isn’t the spirit of what the dev policy is talking about.  Reversion is talking about a specific patch in question, not about removing or changing code.   I don’t think anyone would reasonably consider Michael’s patch to be a patch reversion in the sense that the dev policy means.

Well, other than the change in file, it contains a almost perfect
revert of r242676, but that I agree that it is probably beside the
point. The main issue with r260488 is that it mixed very desirable
direct improvements with unrelated features and regressions.
Reverting, splitting and committing again is IMHO the only way to make
it clear which is which.

Cheers,
Rafael