[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:129
 
+    /// Gets next index that hasn't been removed after \param CurrentIndex.
+    uint64_t getNextIndex(uint64_t CurrentIndex) const {
----------------
You've lost the "the" between "Gets" and "next".

This part hasn't been addressed:
> 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
+    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) {
----------------
Why the cast? And what is the type of `Range`? I don't think you want `auto` here.


================
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;
----------------
What happens if `std::find_if` returns `Range.end()`? Won't that result in you dereferencing something invalid?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:152
+
+    /// different to size() because size() returns the number of entries that
+    /// haven't been removed.
----------------
You misunderstood my previous comment. I only wanted you to change the comment from the word "different". In other words, the full text should be:

```
    /// Returns the index of the last element. This is different to size()
    /// because size() returns the number of entries that haven't been removed.
```


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


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:482-483
+
+  // 4 additional sections, index 0, .symtab, .strtab and .shstrtab are added
+  // yaml2obj.
+  EXPECT_EQ(
----------------
added by yaml2obj


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

https://reviews.llvm.org/D65633