[PATCH] D50381: [llvm-objcopy] Add --prefix-symbols option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:215
 
+static std::vector<std::unique_ptr<std::string>> SymbolsPrefixedNames;
+
----------------
paulsemel wrote:
> jhenderson wrote:
> > paulsemel wrote:
> > > This might not be the correct way to retain the new symbols names (and surely not the correct place to have it anyway), but I couldn't find a smarter way to do it..
> > The lazy answer would be to just give ownership of symbol names to the symbol class. I don't know if that's the right thing to do, but it feels a bit better than this to me.
> Well, not really, as a symbol shouldn't have anything else than a StringRef (as it is to be immutable). The fact is that we need to have a "new" string in memory, to be referenced by this symbol.. 
But, symbol names are not immutable (as demonstrated by the fact that we have this option). I don't see what StringRef gives us here, apart from a bit of efficiency for cases when we don't need to change the symbol name.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:372
+        SymbolsPrefixedNames.emplace_back(
+            new std::string(Config.SymbolsPrefix.str() + Sym.Name.str()));
+        Sym.Name = *SymbolsPrefixedNames.back();
----------------
paulsemel wrote:
> jhenderson wrote:
> > There's not much point in newing a std::string into existence. Just create a vector of strings.
> No, with vector of strings, the address can change (using vectors, we can't ensure the address of elements will remain the same)
Fair (although pre-allocating the vector capacity would solve this issue too). But this becomes moot if we give ownership of names to the symbols...


Repository:
  rL LLVM

https://reviews.llvm.org/D50381