[PATCH] D66070: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 6]

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:527
+MutableELFSegment<ELFT> &
+MutableELFObject<ELFT>::makeMutableSegment(uint64_t Seg) {
+  if (MutableELFSegment<ELFT> *Phdr = Segments.getIfNew(Seg))
----------------
Can this be inlined into getMutableSegment?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:528
+MutableELFObject<ELFT>::makeMutableSegment(uint64_t Seg) {
+  if (MutableELFSegment<ELFT> *Phdr = Segments.getIfNew(Seg))
+    return *Phdr;
----------------
Is there a particular reason this implementation requires `getIfNew`, whereas the symbol and section versions don't?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:808
+  MutableELFSegment<ELF64LE> &MutSegment = *MutSegmentOrErr;
+  MutSegment.Header.p_vaddr = 0x1100;
+  EXPECT_EQ(FirstSegment->getHeader().p_vaddr, 0x1100u);
----------------
Changing the address of a segment is very unlikely to ever actually happen, I'd think. I'd change the p_offset field instead.


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

https://reviews.llvm.org/D66070