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

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:131
+    uint64_t getNextIndex(uint64_t CurrentIndex) const {
+      auto Range = seq(CurrentIndex + 1, (uint64_t)Mappings.size());
+      return *std::find_if(Range.begin(), Range.end(), [this](uint64_t Index) {
----------------
jhenderson wrote:
> Why the cast? And what is the type of `Range`? I don't think you want `auto` here.
Because templates are very finicky :) The alternative is I could do `seq<uint64_t>(CurrentIndex + 1, Mappings.size())`. Do you have a preference either way?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:132
+      auto 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;
----------------
jhenderson wrote:
> What happens if `std::find_if` returns `Range.end()`? Won't that result in you dereferencing something invalid?
I had to check this to make sure, `*llvm::seq(0, 10).end()` yields 10, so it just returns the index in the end state, so just `Mappings.size()`.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:435
+  EXPECT_EQ(*NameOrErr, "first");
+  MutableObject.removeSymbol(FirstSym);
+
----------------
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?


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

https://reviews.llvm.org/D65633