<div dir="ltr"><div>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:</div><div><br></div><div>auto ErrOrItr = File->error_or_child_begin();</div><div>if (!ErrOrItr)</div><div> return ErrOrItr.getError();</div><div>auto Itr = *ErrOrItr;</div><div>while (Itr != file->child_end()) {</div><div> auto &Thing = *Itr;</div><div><br></div><div> ErrOrItr = increment(Itr);</div><div> if (!ErrOrItr)</div><div> return ErrOrItr.getError();</div><div>}</div><div><br></div><div>If you write an iterator wrapper that includes a failure state you can get that down to:</div><div><div><br></div><div>auto ErrOrItr = File->child_begin();</div><div>for (; ErrOrItr && ErrOrItr != File->child_end(); ErrOrItr.next()) {</div><div> Child = *ErrOrItr;</div><div> // ...</div><div>}</div><div>if (!ErrOrItr)</div><div> return ErrOrItr.getError();</div></div><div><br></div><div>But now you have to be careful to check the error outside the loop.</div><div><br></div><div>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.</div><div><br></div><div>- Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 29, 2015 at 8:30 PM, Kevin Enderby <span dir="ltr"><<a href="mailto:enderby@apple.com" target="_blank">enderby@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Lang and others,<div><br></div><div>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):</div><div><br></div><div>for (const Archive::Child &Child : File->children()) {</div><div> …</div><div><br></div><div>to this:</div><div><br></div><div> for (auto &ChildOrErr : File->children()) {<br> error(ChildOrErr, "Could not get the child of the archive " +<br> File->getFileName());<br> const Archive::Child Child(*ChildOrErr);</div><div> ...</div><div><br></div><div>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.</div><div><br></div><div>My thoughts,</div><div>Kev</div><div><div class="h5"><div><br></div><div><div><blockquote type="cite"><div>On Oct 29, 2015, at 7:50 PM, Lang Hames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br><div><div dir="ltr">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.<div><div><br></div><div>I don't have particularly strong feelings either way.</div><div><br></div><div>- Lang.</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 29, 2015 at 5:51 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Oct 29, 2015 at 5:27 PM, Lang Hames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Rafael,<div><br></div><div>How could you ignore the error in Kevin's solution?</div><div><br></div><div>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.</div></div></blockquote><div><br></div></span><div>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.<br><br>I can't find the example you guys had:<br><br>for (auto &Thing : things) {<br> if (!Thing)<br> return Thing.getError();<br> ...<br>}<br><br>& yeah, nothing that comes to mind is as tidy/simple as that, my nearest might be:<br><br>auto Iter = things.begin();<br>while (auto &Thing = Iter.Next()) {<br> ....<br>}<br>if (!Iter)<br> return Iter.getError();<br><br>(where "Iter" isn't a traditional iterator in any sense - it has ErrorOr<T> Next() and getError (& possibly boolean testability))<br><br>The obvious sort of problem I imagine arising from the iterator/range API is something like:<br><br> int count = std::distance(things.begin(), things.end());<br><br>Easy to write, and would produce an infinite loop (or crash?) if there was an error during iteration.<br><br>- Dave<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><div dir="ltr"><div><br></div><div>Cheers,</div><div>Lang.</div><div><div><div><br></div><div><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 29, 2015 at 5:11 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On 28 October 2015 at 15:21, Kevin Enderby <<a href="mailto:enderby@apple.com" target="_blank">enderby@apple.com</a>> wrote:<br>
> enderby added a comment.<br>
><br>
> 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 <a href="http://reviews.llvm.org/D13990" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13990</a> .<br>
<br>
</span>I think that the most important consideration is making the error hard<br>
to ignore. That means using ErrorOr.<br>
<br>
The sad reality is that the archive format can cause us to find an<br>
error when going from one member to the next. That, with the above<br>
requirement means that it doesn't fit on range loop.<br>
<span><br>
> 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.<br>
><br>
> F1026846: llvm_updated_t.diff <<a href="http://reviews.llvm.org/F1026846" rel="noreferrer" target="_blank">http://reviews.llvm.org/F1026846</a>><br>
<br>
<br>
</span>Thanks!<br>
<br>
You don't need to change ErrorOr.h.<br>
<br>
The change to detect the end of the archive seems more complicated<br>
than necessary (see the attached patch for a suggestion).<br>
<br>
Cases like the Increment in BinaryHolder expose a naked EC, which is<br>
what using ErrorOr is trying to avoid. Just inlining it solves the<br>
problem (see patch).<br>
<br>
Please convert the remaining user of Increment to return void when possible.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div></div></div></div>
<br></span><span>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></span></blockquote></div><br></div></div>
</blockquote></div><br></div>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>