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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 3 inline comments as done.
mstorsjo added a comment.

In D55881#1353276 <https://reviews.llvm.org/D55881#1353276>, @rupprecht wrote:

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


Ok - should I still wait for @jakehehrlich, or do you happen to know about his whereabouts?



================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:20
+void Object::addSymbols(ArrayRef<Symbol> NewSymbols) {
+  for (Symbol S : NewSymbols) {
+    S.UniqueId = NextSymbolUniqueId++;
----------------
rupprecht wrote:
> `Symbol& S`?
No, intentionally not a reference here. The point is that we have an `ArrayRef<Symbol>` from the caller, which essentially is read-only, and we mutate the Symbol objects as we insert them into the Object class' internal vector.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:31
+  for (Symbol &Sym : Symbols) {
+    SymbolMap[Sym.UniqueId] = &Sym;
+    Sym.RawIndex = RawSymIndex;
----------------
rupprecht wrote:
> 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())`.
Ok, I'll try that.


================
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;) {
----------------
rupprecht wrote:
> Similarly you can set an initial size of `COFFObj.getRawNumberOfSymbols()` here
Ok, will do. That'd be `Symbols.reserve(COFFObj.getRawNumberOfSymbols())` right, as it can't be done via the constructor directly?


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

https://reviews.llvm.org/D55881