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

[David Blaikie via llvm-commits]

On Thu, Oct 29, 2015 at 5:27 PM, Lang Hames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

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

I'm not taking a position either way - but that does present a difficult
problem for the iterator contract, to say that you can't increment it twice
without dereferencing... that's not really in the iterator
playbook/contract (could implement a little extra support for that by
allowing an error-state iterator to be incremented to become the end
iterator). I know you've mentioned it before (the "can't increment twice
without checking") but it became a bit clearer to me in this recent email
exchange to the point that I would be a little less comfortable with it.

I can't find the example you guys had:

for (auto &Thing : things) {
  if (!Thing)
    return Thing.getError();
  ...
}

& yeah, nothing that comes to mind is as tidy/simple as that, my nearest
might be:

auto Iter = things.begin();
while (auto &Thing = Iter.Next()) {
  ....
}
if (!Iter)
  return Iter.getError();

(where "Iter" isn't a traditional iterator in any sense - it has ErrorOr<T>
Next() and getError (& possibly boolean testability))

The obvious sort of problem I imagine arising from the iterator/range API
is something like:

  int count = std::distance(things.begin(), things.end());

Easy to write, and would produce an infinite loop (or crash?) if there was
an error during iteration.

- Dave

- Dave


>
> 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
>>
>
>
> _______________________________________________
> 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/20151029/690f7444/attachment.html>