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

[Lang Hames via llvm-commits]

Hi Rafael,

How could you ignore the error in Kevin's solution?

I think your solution and his are essentially the same, except that you
check the error on increment and he checks it on dereference. In practice
you always have to do both operations (you can't do multiple increments
without checking the error anyway, and you can just do that by deref'ing),
and checking via deref allows you to used range-based for, which makes the
code neater.

Cheers,
Lang.

On Thu, Oct 29, 2015 at 5:11 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 28 October 2015 at 15:21, Kevin Enderby <enderby at apple.com> wrote:
> > enderby added a comment.
> >
> > I agree with Lang and like the original patch where were were handling
> failure was done inside the iterator.  It does allow use of ranged-based
> for loops and appears to me much cleaner in the code especially in the lld
> changes needed in http://reviews.llvm.org/D13990 .
>
> I think that the most important consideration is making the error hard
> to ignore. That means using ErrorOr.
>
> The sad reality is that the archive format can cause us to find an
> error when going from one member to the next. That, with the above
> requirement means that it doesn't fit on range loop.
>
> > Rafael, I did "finish it up" based on your t.diff patch and that is
> below and includes the re-formatting with clang-format-diff, removal of the
> malformed-archives directory putting the .a files in Inputs and removal of
> the "nice but independent" changes.
> >
> > F1026846: llvm_updated_t.diff <http://reviews.llvm.org/F1026846>
>
>
> Thanks!
>
> You don't need to change ErrorOr.h.
>
> The change to detect the end of the archive seems more complicated
> than necessary (see the attached patch for a suggestion).
>
> Cases like the Increment in BinaryHolder expose a naked EC, which is
> what using ErrorOr is trying to avoid. Just inlining it solves the
> problem (see patch).
>
> Please convert the remaining user of Increment to return void when
> possible.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151029/315178c7/attachment.html>