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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Self note: I still need to look at coverage of the unit tests.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:74
+  /// iterating over the MutableTable. Notably removed values stay in the table
+  /// to preserve the index of all other other entries, similarly insertion is
+  /// not supported.
----------------
entries, similarly -> entries. Similary


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:128
 
+    /// Gets the next non remove index after \param CurrentIndex.
+    uint64_t getNextIndex(uint64_t CurrentIndex) const {
----------------
non remove index -> next index that hasn't been removed

The comment should say what happens if there are no such indexes before the end of the table.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:131
+      // <= so that Index can reach an end iterator state.
+      while (Mappings[++CurrentIndex].Type == MappingType::Removed &&
+             CurrentIndex <= Mappings.size())
----------------
You need to assert that CurrentIndex is valid.

std::find_if might be an easier to read function to use here.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:138
+    /// Returns the first index which has not been removed.
+    uint64_t getFirstIndex() const {
+      for (uint64_t I = 0; I != Mappings.size(); ++I)
----------------
Could this be based on `getNextIndex()`? You could check first if the first element has not been removed, and then call `getNextIndex(0);`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:145
+
+    /// Counts the number of non removed entries between 0 and \param Index.
+    uint64_t getRelativeIndex(uint64_t Index) const {
----------------
non removed entries -> entries that haven't been removed


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:146
+    /// Counts the number of non removed entries between 0 and \param Index.
+    uint64_t getRelativeIndex(uint64_t Index) const {
+      uint64_t Ret = 0;
----------------
Maybe `getEffectiveIndex`? Relative doesn't really sound right. What is it relative to?


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


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:156
+    /// because size returns the number of valid (non removed) entries.
+    size_t getLastIndex() const { return Mappings.size(); }
+
----------------
Perhaps call this `getEndIndex()`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:344
+  /// Removes a symbol.
+  void removeSymbol(symbol_iterator Sym) { removeSymbol(*Sym); }
+
----------------
Superfluous overload.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:352
+  /// Removes a section.
+  void removeSection(section_iterator Sec) { removeSection(*Sec); }
 };
----------------
Superfluous overload.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:402
+
+// Test removeSymbol method
+TEST(MutableELFObject, RemoveSymbols) {
----------------
Missing trailing full stop.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:427
+
+  auto Distance =
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end());
----------------
This shouldn't be `auto`. Can it be done inline in the check?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:432
+  auto FirstSym = ++MutableObject.symbol_begin();
+  auto NameOrErr = symbol_iterator(FirstSym)->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
----------------
This shouldn't be `auto`.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:443
+  Distance =
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end());
+  EXPECT_EQ(Distance, 2);
----------------
Can this be done inline in the check?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:447
+
+// Test removeSection method
+TEST(MutableELFObject, RemoveSections) {
----------------
Missing trailing full stop.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:470
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      6);
+
----------------
Probably requires a comment saying why "6".


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:483
+
+  // At this point Iter is on a removed index, it is invalid and any method
+  // should abort (from an assert). If for some reason tests are being run with
----------------
"index. It"

But really, I'm not sure you should have these tests here. We don't usually test assertions. If this a likely situation to occur, then perhaps the methods should return an `Expected`.


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

https://reviews.llvm.org/D65633