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

Thu Oct 29 22:53:19 PDT 2015

That's a good point: next() has to return an ErrorOr<iterator>, and you
have to distinguish between failure and end(), so if you want to check each
call to next() you need to do something like:

auto ErrOrItr = File->error_or_child_begin();
if (!ErrOrItr)
  return ErrOrItr.getError();
auto Itr = *ErrOrItr;
while (Itr != file->child_end()) {
  auto &Thing = *Itr;

  ErrOrItr = increment(Itr);
  if (!ErrOrItr)
    return ErrOrItr.getError();
}

If you write an iterator wrapper that includes a failure state you can get
that down to:

auto ErrOrItr = File->child_begin();
for (; ErrOrItr && ErrOrItr != File->child_end(); ErrOrItr.next()) {
  Child = *ErrOrItr;
  // ...
}
if (!ErrOrItr)
  return ErrOrItr.getError();

But now you have to be careful to check the error outside the loop.

Looking at Kevin's updated diff, the change to the iterators made those
loops a lot uglier. The introduction of the Increment function fixes some
of that, but at the cost of risking the problem that you were trying to
avoid: It's suddenly very easy to "increment" the iterator without actually
checking the error.

- Lang.

On Thu, Oct 29, 2015 at 8:30 PM, Kevin Enderby <enderby at apple.com> wrote:

> Hi Lang and others,
>
> This iterator basically requires another indirection to get to the
> underlying Child.  So you get a compile error when trying to use
> the iterator as it is an <ErrorOr>.  So the example really is to change
> something like this (as the diff in ELF/InputFiles.cpp):
>
> for (const Archive::Child &Child : File->children()) {
>   …
>
> to this:
>
>  for (auto &ChildOrErr : File->children()) {
>     error(ChildOrErr, "Could not get the child of the archive " +
>                           File->getFileName());
>     const Archive::Child Child(*ChildOrErr);
>   ...
>
> Which seems a bit cleaner than testing the result of child_begin() before
> the for loop and checking getNext() even if it can be wrapped in some case
> in and Increment() routine.
>
> My thoughts,
> Kev
>
> On Oct 29, 2015, at 7:50 PM, Lang Hames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Yep. The standard C++ iterator concept doesn't apply here, since it
> doesn't deal with failure, and checking on deref rather than increment
> means you can accidentally do multiple increments without checking. My
> intent was to have increment crash if you run it on an error'd out
> iterator, but that's not as good as a compile-time check. I think it's a
> matter of personal taste whether the cost (IMHO fairly minimal, since in
> practice you almost always deref after increment) is worth paying for the
> gain (also arguably minimal) of a neater iteration idiom.
>
> I don't have particularly strong feelings either way.
>
> - Lang.
>
> On Thu, Oct 29, 2015 at 5:51 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> 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
>>>
>>>
>>
> _______________________________________________
> 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/aa9003af/attachment.html>