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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm out of time for today on this review. I'll come back to it tomorrow.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:535-536
+  uint64_t Index = Sec.getRawDataRefImpl().p;
+  if (const MutableELFSection<ELFT> *Section = Sections.getConstIfNew(Index))
+    return Section->ParentIndex;
+
----------------
A comment briefly explaining this would be helpful.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:541-544
+    if (sectionWithinSegment(Shdr, Phdr)) {
+      Optional<size_t> ParentsParent = findParentSegment(SegmentRef(this, I));
+      return ParentsParent ? *ParentsParent : I;
+    }
----------------
Some code comments explaining what is going on here would be useful.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:553-554
+  size_t Index = Segment.getIndex();
+  if (const MutableELFSegment<ELFT> *Segment = Segments.getConstIfNew(Index))
+    return Segment->ParentIndex;
+
----------------
An explanation with some comments as to why you can have this early out would make this clearer.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:593
+bool MutableELFObject<ELFT>::segmentWithinSegment(
+    const typename ELFT::Phdr &Segment, size_t ChildOffset,
+    const typename ELFT::Phdr &PotentialParent, size_t ParentOffset) {
----------------
ChildOffset -> SegmentIndex
ParentOffset -> ParentIndex

Write a comment at the start of this function outlining the behaviour, please.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:923-924
+    Align:      1
+  - Type:       4         # Reorder 4 and 5 so 4 doesnt come before and become
+    Offset:     0x4001    # the parent just because it came first in the loop.
+    FileSize:   0x100
----------------
doesnt -> doesn't

But really, I'm a little confused what this comment is trying to say. I think you want to say something about offsets, because it's confusing what you're talking about in this comment. It appears you're saying 4 doesn't come before 5, but it's earlier in the program header list, which seems to contradict what you're saying.


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

https://reviews.llvm.org/D66070