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

[Sean Silva via llvm-commits]

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

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

-- Sean Silva


> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160216/aa446037/attachment.html>