[PATCH] D43958: [llvm-readobj][ELF] Move ELF note parsing into lib/Object

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: include/llvm/Object/ELFTypes.h:647
+    assert(Start && "ELF note iterator starting at NULL");
+    assert(!Err && "ELF note iterator starting with an Error");
+    if (RemainingSize == 0)
----------------
scott.linder wrote:
> t-tye wrote:
> > Since this iterator constructor always requires an Err should it be passed as Error&?
> This was a typo, I was also intending to check the `bool` of the `Error`, not just the `Error *`. Thank you for the catch, I will update the patch.
> 
> There are cases (such as constructing the end iterator) where there is no chance for error, and so having a non-nullable reference is not appropriate (and complicates the calling code for no benefit). The alternative is an `Optional<Error &>`, which I can update the patch to use. I could also do the same for the pointer to the `Nhdr`, and in both cases it would make the fact that they are optional clearer.
I think I understand what you mean after re-reading your comment; I can certainly make the constructor accept the reference and take the pointer itself, so I will make that change at least.


https://reviews.llvm.org/D43958