[PATCH] D22079: Refactor Archive-child iteration.

[Rafael Espíndola via llvm-commits]

OK, so the nullptr case is just for the end iterator? If so, can you
try to change the constructors to

private:
   child_iterator() : C(Child(nullptr, nullptr, nullptr)), E(nullptr) {}
public:
    child_iterator(const Child &C, Error &E) : C(C), E(&E) {}

Thanks,
Rafael



On 7 July 2016 at 14:34, Lang Hames <lhames at gmail.com> wrote:
> lhames added inline comments.
>
> ================
> Comment at: include/llvm/Object/Archive.h:132
> @@ -132,3 +131,3 @@
>      child_iterator &operator++() { // Preincrement
> -      assert(child && "Can't increment iterator with error");
> -      child = child->getNext();
> +      assert(E && "Can't increment iterator with no Error attached");
> +      if (auto ChildOrErr = C.getNext())
> ----------------
> rafael wrote:
>> and this assert?
>>
>> Basically this code has the invariant that E can be null, but we know it is not when needed, which is non-obvious.
>> Basically this code has the invariant that E can be null, but we know it is not when needed, which is non-obvious.
>
> Perhaps some extra documentation in the class header would help, but for clients nothing really changes here: child_begin now takes an Error& and will use this to report error if increment fails. The child_end() method doesn't take an Error&, and the returned iterator can't be incremented, but that was already true -- it would (hopefully) have segfaulted in Child::getNext when we try to access the null-initialized Data member.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D22079
>
>
>