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

[Alex Brachet via Phabricator via llvm-commits]

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


================
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;
----------------
rupprecht wrote:
> 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.
FWIW, I think `size` is the only method which gets its complexity changed, this can be solved by just keeping a running count, and having `add` and `remove` methods change `Size`. Does that sound ok?

Methods like `getMutableSection` index into the table without any checks to deleted entries so those are all O(1), I'm trying to think of others which became O(N^2) but I cannot think of them.


================
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);
----------------
rupprecht wrote:
> 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.
I created this `MatchesRange` template function. It's not as cool as what you were suggesting but I couldn't figure out how to make a gtest matcher.


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

https://reviews.llvm.org/D65633