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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 9 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:72
+
+  MutableELFSegment(Elf_Phdr Header) : Header(Header) {}
+
----------------
MaskRay wrote:
> The ctor is not really necessary.
It eventually gets called by an `emplace_back`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:211
 
+    NewType *getIfNew(uint64_t Index) {
+      assert(Index < Mappings.size() && "Out of bounds");
----------------
MaskRay wrote:
> This should be added to Part I D64281.
I had this in an earlier patch but then moved it out because it wasn't needed until now. I don't have a preference either way which patch it goes in.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:302
+  public:
+    SegmentRef(MutableELFObject<ELFT> *Object, uint64_t Index = 0)
+        : Object(Object), Index(Index) {}
----------------
MaskRay wrote:
> You can use `uint64_t Index = 0; ` (did you mean `size_t Index = 0;`?) and delete the ctor.
Hmm am I missing something on how to construct objects? C++ doesn't have the nice C syntax like `SegmentRef S = {.Object = nullptr, .Index = 0};` Is there a way to leave out these boiler plate constructors that I don't know of?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:528
+MutableELFObject<ELFT>::makeMutableSegment(uint64_t Seg) {
+  if (MutableELFSegment<ELFT> *Phdr = Segments.getIfNew(Seg))
+    return *Phdr;
----------------
jhenderson wrote:
> Is there a particular reason this implementation requires `getIfNew`, whereas the symbol and section versions don't?
Yes, although I'm not sure if you'll like it :) Creating a `MutableELFSegment` is more expensive than the others because it takes a list of all sections that reside in it. So using `getIfNew` allows to not have to calculate that if it is already been made mutable then it just returns a mutable reference to a `MutableELFSegment`. I say you might not like that because what I described doesn't happen in this patch but only will in Part 7.


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

https://reviews.llvm.org/D66070