[PATCH] D55881: [llvm-objcopy] [COFF] Add support for removing symbols

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht added a comment.

Just some nits, but I think this is good to commit.



================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:20
+void Object::addSymbols(ArrayRef<Symbol> NewSymbols) {
+  for (Symbol S : NewSymbols) {
+    S.UniqueId = NextSymbolUniqueId++;
----------------
`Symbol& S`?


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:31
+  for (Symbol &Sym : Symbols) {
+    SymbolMap[Sym.UniqueId] = &Sym;
+    Sym.RawIndex = RawSymIndex;
----------------
Given the number of symbols may be large and you know the exact size you want to add, you might want to avoid reallocation by changing `SymbolMap.clear()` with `SymbolMap = DenseMap<xxx>(Symbols.size())`.


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:87
 Error COFFReader::readSymbols(Object &Obj, bool IsBigObj) const {
+  std::vector<Symbol> Symbols;
   for (uint32_t I = 0, E = COFFObj.getRawNumberOfSymbols(); I < E;) {
----------------
Similarly you can set an initial size of `COFFObj.getRawNumberOfSymbols()` here


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

https://reviews.llvm.org/D55881