[llvm] r256156 - [llvm-objdump] Use appropriate helper. NFC.

[Davide Italiano via llvm-commits]

On Tue, Jan 12, 2016 at 3:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Jan 12, 2016 at 3:20 PM, Davide Italiano <davide at freebsd.org> wrote:
>>
>> On Tue, Jan 12, 2016 at 2:54 PM, Davide Italiano <davide at freebsd.org>
>> wrote:
>> > On Tue, Jan 12, 2016 at 2:24 PM, David Blaikie <dblaikie at gmail.com>
>> > wrote:
>> >>
>> >>
>> >> On Tue, Jan 12, 2016 at 11:57 AM, Davide Italiano <davide at freebsd.org>
>> >> wrote:
>> >>>
>> >>> On Tue, Jan 12, 2016 at 11:45 AM, David Blaikie <dblaikie at gmail.com>
>> >>> wrote:
>> >>> >
>> >>> >
>> >>> > On Mon, Dec 21, 2015 at 6:10 AM, Davide Italiano via llvm-commits
>> >>> > <llvm-commits at lists.llvm.org> wrote:
>> >>> >>
>> >>> >> Author: davide
>> >>> >> Date: Mon Dec 21 08:10:54 2015
>> >>> >> New Revision: 256156
>> >>> >>
>> >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=256156&view=rev
>> >>> >> Log:
>> >>> >> [llvm-objdump] Use appropriate helper. NFC.
>> >>> >
>> >>> >
>> >>> > Test case?
>> >>> >
>> >>>
>> >>> test/tools/llvm-objdump/invalid-input.test should somehow cover this,
>> >>> I guess.
>> >>
>> >>
>> >> But there were no changes for that test when this commit was made - so
>> >> it
>> >> doesn't seem like it would be testing the change you made (in the sense
>> >> that
>> >> it wouldn't catch a regression caused by reverting this patch).
>> >>
>> >> What would fail if I revert this patch or otherwise broke the
>> >> functionality
>> >> you were fixing in this commit?
>> >>
>> >
>> > hmm, I understand you concerns, sorry if it came out as I didn't.
>
>
> Ah, sorry for my misunderstanding.
>
>>
>> > I'll try to build a broken MachO executable and check that in.
>> >
>> >
>>
>> OK, it seems code is simply dead (and therefore could be removed).
>>
>> In particular:
>> * If -macho is specified, we already bail out correctly with invalid
>> object file much earlier:
>>
>> (gdb)
>> 1480      if (std::error_code EC = BinaryOrErr.getError()) {
>> (gdb)
>> 1481        errs() << "llvm-objdump: '" << Filename << "': " <<
>> EC.message() << ".\n";
>> (gdb)
>> llvm-objdump: 'hello-macho.bin.2': The file was not recognized as a
>> valid object file.
>>
>> * If the object is valid (but not MachO), that's covered by the if
>> statement above the line I changed in the commit.
>>
>> Both of these are, alas, untested.
>> I propose to:
>> 1) Add test for these
>> 2) Remove the whole else {} condition I previously changed.
>
>
> Based on your description that sounds plausible to me (if you want to be a
> bit more sure, you could try adding an assert/llvm_unreachable there either
> temporarily or permanently for the case that seems to have become dead -
> that way if it turns out not to be dead, we'll hopefully hear about it & can
> add new tests/code to cover it then)
>
> Will leave it to you (& Kevin/etc) to decide & just go back to reading this
> thread.
>

r257561 and r257570. Thank you for your patience!

--
Davide