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

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: include/llvm/Object/ELFTypes.h:621
+      : Nhdr(From), To(To), Err(Err) {
+    checkBounds();
+  }
----------------
To be consistent with operator++ suggest:

```
if (From == To)
  Nhdr = nullptr;
else
  checkBounds();
```

This supports iterating over any empty notes list by ensuring the returned iterator is the end iterator (represented as Nhdr == nullptr).

Technically setting Nhdr to pointing outside an object or one past the end of an object is not defined in C++. So really should be checking the sizes before creating the pointers.


================
Comment at: include/llvm/Object/ELFTypes.h:625
+public:
+  Elf_Nhdr_Iterator_Impl &operator++() {
+    const uint8_t *NhdrPosition = reinterpret_cast<const uint8_t *>(Nhdr);
----------------
Suggest add an assert for Nhdr being nullptr. That ensures are not incrementing an end iterator. This also avoids checkBounds having to check Err is nullptr as the only way for that to happen is by constructing an end iterator.


================
Comment at: include/llvm/Object/ELFTypes.h:641-642
+  }
+  const Elf_Nhdr_Impl<ELFT> *operator*() const { return Nhdr; }
+};
+
----------------
Suggest this return a reference type not a pointer, and asserts if Nhdr is nullptr indicating the end iterator.


https://reviews.llvm.org/D43958