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

[Kevin Enderby via llvm-commits]

> On 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.
>> 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.
> 
> Also, CC:ing Kevin Enderby as he knows it much better than I do.
> Kevin, what do you think?

I’m fine with removing the dead code.  Or if you want to use the report_error() helper I’m fine with that too but it seems cleaner to remove the dead code and add a test to make sure the error is caught through this path.

> 
> -- 
> Davide
> 
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare