[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:794
+template <class ELFT>
+bool ELFObjectFile<ELFT>::isBerkeleyText(const Elf_Shdr *Shdr) {
+  return Shdr->sh_flags & ELF::SHF_ALLOC &&
----------------
I'm not sure I understand why you've made this change from the previous version? I don't think it gives anything.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:17
+// taking an Iterable::value_type.
+template <typename T> class MutableRange {
+public:
----------------
abrachet wrote:
> jhenderson wrote:
> > This is the only class in this file, so the file name seems a bit off?
> Not sure what to do here. I think eventually I will have a MutableObject base class, and it would seem silly to make a MutableRange.h. What do you think I should do?
I'd probably put it in MutableELFObject.h for now and move it out in a later change when needed.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:43
+
+  auto getSectionName = [](section_iterator Iter) -> const char * {
+    StringRef Name;
----------------
This still has the unnecessary trailing return type.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:94
+  MutableELFObject<ELF64LE> MutableObject(*ELFObjFile);
+
+  auto FirstSec = ++MutableObject.section_begin();
----------------
You need an ASSERT here that there are the correct number of sections.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64281/new/

https://reviews.llvm.org/D64281