[PATCH] D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2]

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments/suggestions are below.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:98
+      std::advance(It, Index);
+      Mappings.erase(It);
+    }
----------------
`Mappings.erase(Mappings.begin() + Index)`?

Perhaps also worth adding an assert like was done above.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:103
+      NewValues.push_back(New);
+      Mappings.push_back(MappingType(NewValues.size() - 1, MappingType::New));
+    }
----------------
```
Mappings.push_back({NewValues.size() - 1, MappingType::New});
```


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:170
+  ArrayRef<Elf_Sym> findSymbolTable(uint64_t ShType) const {
+    for (const auto &Sec : this->sections())
+      if (ELFSectionRef(Sec).getType() == ShType)
----------------
You should probably at least add an assert checking `ShType` is one of the symbol table types.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:177
+
+    return None;
+  }
----------------
`return {};`
(Its a bit confusing to see `None` which is normally used for returning the empty `Optional<>`)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:180
+
+  uint32_t findSymbolTableIndex(uint64_t ShType) const {
+    for (uint32_t I = 0; I < Sections.originalSize(); ++I)
----------------
It could be `findSectionIndex`, since you nowere check that `ShType` is a symbol table.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:197
+               : DynSymbols;
+  }
+
----------------
What about implementing `const` version using `non-const`? I.e. call `getWhichTable` from `getWhichTable const`.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:263
+  /// Removes either a dynamic symbol or regular symbol.
+  void removeSymbol(symbol_iterator Sym) {
+    MutableSymbolTable &SymTab = getWhichTable(Sym->getRawDataRefImpl());
----------------
I think when you have a symbol iterator, you usually know if it is a regular or dynamic
symbol. I am not sure it worth to allow mixing them. I.e. I am thinking about making 2 different functions,
like `removeSymbol` and `removeDynamicSymbol`.

I am not feeling strong here, though it seems to be less error prone.


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

https://reviews.llvm.org/D65367