[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: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;
----------------
abrachet wrote:
> 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.
Keeping a running count would make sense to me, if it improves the complexity of other functions.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:129-131
+    /// Gets the next index that hasn't been removed after \param CurrentIndex.
+    /// If there is not an index after \param CurrentIndex which has not been
+    /// removed, this returns the index past the last entry.
----------------
Hmm... thinking about it, how about "Gets the index of the next entry that hasn't ..." and "If there is no entry after ..."

Also "this returns the index one past the last entry" to be more precise.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:134
+      assert(CurrentIndex < Mappings.size());
+      return *llvm::find_if(seq(CurrentIndex + 1, (uint64_t)Mappings.size()),
+                            [this](uint64_t Index) {
----------------
Maybe the cast indicates that these indexes should be size_t everywhere, since they're indexes into a container? What do you think?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:435
+  EXPECT_EQ(*NameOrErr, "first");
+  MutableObject.removeSymbol(FirstSym);
+
----------------
abrachet wrote:
> jhenderson wrote:
> > abrachet wrote:
> > > jhenderson wrote:
> > > > I think it would be interesting to see what happens when you remove the second symbol too, leaving only the null entry. I also think it would be interesting to test what happens if you try to remove the null entry (I think this should probably be an error).
> > > I haven't done this or the similar comment for sections yet. Do you think the `removeSymbol` method should return `Error` or should this just be an `assert`?
> > Good question. I'd probably just `assert` for now.
> Ok, I've added assertions to `removeSymbol` and `removeSection`. I should not test these explicitly because it is an `assert`, right?
Exactly. Don't test asserts.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:23
+template <typename Iter, typename T = typename Iter::value_type,
+          bool MatchLen = true>
+bool MatchesRange(
----------------
Do you need the MatchLen parameter? I don't think you do...


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:28
+  if (MatchLen)
+    if ((size_t)std::distance(Begin, End) != List.size())
+      return false;
----------------
`std::distance` already returns a size_t. No need to cast it.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:31
+
+  for (T Value : List) {
+    if (Value != Extractor(Begin))
----------------
`const T&` maybe?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:509
+
+  bool Match = MatchesRange<section_iterator, StringRef>(
+      MutableObject.section_begin(), MutableObject.section_end(),
----------------
Match -> SectionsMatch


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

https://reviews.llvm.org/D65633