[PATCH] D13989: This removes the eating of the error in Archive::Child::getSize() when the charactersin the size field in the archive header for the member is not a number.

[Kevin Enderby via llvm-commits]

Hello Rafael,

Here are the updated diffs that I think will address the items you pointed out below.

Kev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: updated_audited_t.diff
Type: application/octet-stream
Size: 31360 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151104/d8f71d7e/attachment.obj>
-------------- next part --------------


> On Nov 4, 2015, at 11:55 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> objdump is still exiting with 0 on error:
> 
> +  if (Size.getError())
> +    outs() << "bad size"
> +           << " ";
> 
> 
> +    if (std::error_code EC = ErrorOrChild.getError()) {
> +      report_error(a->getFileName(), EC);
> +      break;
> +    }
> 
> You don't need a break after report_error. Without it you also don't
> need the {}.
> 
> Similar case in:
> 
>    if (std::error_code EC = ErrorOrChild.getError()) {
> +      reportError(Arc->getFileName(), EC.message());
> +      break;
> +    }
> 
> 
> llvm-size still exits with zero on error:
> 
> +      if (i->getError()) {
> +        errs() << ToolName << ": " << file << ": " << i->getError().message()
> +               << ".\n";
> +        break;
> +      }
> 
> 
> Cheers,
> Rafael
> 
> 
> 
> On 4 November 2015 at 14:51, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>>> On Oct 13, 2015, at 2:23 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>>> 
>>>> Ideally we want to push them up until we get out of lib, at which
>>>> point we are in tools and can handle the error in whatever way is
>>>> appropriate for that tool.
>>> 
>>> 
>>> to mean that once the error was pushed out of the lib the tool could really could handle the error in whatever way is appropriate for that tool, including eating the error and ignoring it.
>> 
>> I honestly don't think that is appropriate. Ever.
>> 
>> A broken .o/.a file is a really rare occurrence. Most of the time it
>> will mean the generator and consumer are out of sync. Very
>> infrequently it will mean a hardware error.
>> 
>> Given that it is an error, I think we *must* exit with non zero status.
>> 
>> Given that it is a really uncommon case, I think we should make the
>> handling in tools as simple as possible: if at all possible, just
>> exit(1).
>> 
>> Taking one last look at the patch.
>> 
>> Cheers,
>> Rafael