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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:23
+template <typename ELFT> struct MutableELFSection {
+  using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
+
----------------
You should be able to use the more common:
```
using Elf_Shdr = typename ELFT::Shdr;
```


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:81-84
+    explicit MutableTable(ArrayRef<OrigType> OriginalValues)
+        : OriginalValues(OriginalValues) {
+      uint64_t Count = 0;
+      std::transform(OriginalValues.begin(), OriginalValues.end(),
----------------
"OriginalValues" is overloaded, so it's not clear which one is being referenced in the constructor

A common hacky practice in LLVM is to name the constructor arg "TheOriginalValues" or something -- that's fine to use if you can't come up with any other different name.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:134-135
+
+  using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
+  using Elf_Ehdr = Elf_Ehdr_Impl<ELFT>;
+
----------------
ditto


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:88-89
+        MutableObject.getMutableSection(Sec);
+    if (!Expect)
+      consumeError(Expect.takeError());
+  }
----------------
Is it expected that an error here should not cause the test to fail? I think you want an assertion that Expect doesn't have an error instead.


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

https://reviews.llvm.org/D64281