[PATCH] D55881: [llvm-objcopy] [COFF] Add support for removing symbols
Martin Storsjö via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 10 13:09:22 PST 2019
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
More information about the llvm-commits
mailing list