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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 9 inline comments as done.
abrachet 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 &&
----------------
jhenderson wrote:
> I'm not sure I understand why you've made this change from the previous version? I don't think it gives anything.
Because I didn't know you could do ELFObjectFile<ELFT>::isBerkeleyText() to not call the overrided method! Oops.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:27
+
+template <typename ELFT> class MutableELFSection {
+public:
----------------
grimar wrote:
> Since all members are `public`, should this be a `struct`?
For some reason I thought I remembered the [[ https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords | style guide ]] saying only POD types should be struct and others should be class regardless of access modifiers. 


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:166-170
+  auto MutObjIter = MutableObject.section_begin();
+  for (const auto &ObjFileIter : ObjFile.sections()) {
+    TestIters(ObjFileIter, MutObjIter);
+    ++MutObjIter;
+  }
----------------
labath wrote:
> using `llvm::zip(ObjFile.sections(), MutableObject.sections())` is a bit cleaner way to handle parallel iteration (but don't forget to assert that the range sizes are the same). 
> 
> With something like `EXPECT_THAT(MutableObject.sections(), testing::ContainerEq(ObjFile.sections())` this check would be a one-liner, but it would require a bit more plumbing to make sure the elements are comparable, so it may not be worth it if this is just a one-off thing...
Thanks never knew about zip!


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

https://reviews.llvm.org/D64281