[PATCH] D65633: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 3]

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:155-156
+
+    /// Returns the index of the last element. This is different to size()
+    /// because size() returns the number of entries that haven't been removed.
+    size_t getEndIndex() const { return Mappings.size(); }
----------------
This comment needs updating.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:142-144
+  ASSERT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      7);
----------------
Is this needed now that you have the following lines?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:150-166
 
   auto Iter = MutableObject.section_begin();
   compareSectionName(Iter, nullptr);
 
   compareSectionName(++Iter, ".sec0");
   compareSectionName(++Iter, ".sec1");
   compareSectionName(++Iter, ".sec2");
----------------
abrachet wrote:
> Should I just remove all of this?
It looks to me to be largely redundant now (though you should still keep the "change the section name in the middle" bit).


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:509-511
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      6);
----------------
Do you need this, given you have the previous line?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:523-525
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      5);
----------------
Do you need this?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:533-535
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      4);
----------------
Do you need this?


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

https://reviews.llvm.org/D65633