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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:134-138
+      iterator_range<detail::value_sequence_iterator<uint64_t>> Range =
+          seq(CurrentIndex + 1, (uint64_t)Mappings.size());
+      return *std::find_if(Range.begin(), Range.end(), [this](uint64_t Index) {
+        return Mappings[Index].Type != MappingType::Removed;
+      });
----------------
`*llvm::find_if(seq(CurrentIndex + 1, (uint64_t)Mappings.size()), [this](uint64_t Index) { ... });`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:193-194
     /// Return the number of elements in the table.
-    size_t size() const { return Mappings.size(); }
+    size_t size() const {
+      return llvm::count_if(Mappings, [](MappingType &Mapping) {
+        return Mapping.Type != MappingType::Removed;
----------------
Many of these methods are now linear, meaning this library isn't feasible to operate on files that have a large number of symbols (especially if you end up calling these methods in a loop and go quadratic). I think this is fine to experiment with but should be noted, so it can be fixed before adding uses of it.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:482-486
+  // 4 additional sections, index 0, .symtab, .strtab and .shstrtab are added
+  // by yaml2obj.
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      6);
----------------
I think writing a test matcher would make this more self-documenting than a comment.
e.g.
`EXPECT_THAT(MutableObject, SectionsAre({".remove_me", ..., }));`
which would check both std::distance and each section name by iterating over all sections.

Writing a test helper (instead of a full blown matcher) would also work; you might not need a thorough matcher outside of this test.


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

https://reviews.llvm.org/D65633