[PATCH] D46177: [llvm-objcopy] Add --globalize-symbol option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:323
            (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL)) ||
-          (!Config.SymbolsToLocalize.empty() &&
-           std::find(std::begin(Config.SymbolsToLocalize),
-                     std::end(Config.SymbolsToLocalize),
-                     Sym.Name) != std::end(Config.SymbolsToLocalize)))
+          contains(Config.SymbolsToLocalize, Sym.Name))
         Sym.Binding = STB_LOCAL;
----------------
paulsemel wrote:
> alexshap wrote:
> > empty() was not completely useless here imo  - this flag is probably not the most frequently used flag, thus when it's not specified the "early check" would save us at least 3 function calls per  symbol (at least in the debug builds) (std::begin/end/find)
> Yes, tbh I think you're right on this one, it might somehow save us time !
I think this is a case of premature optimization. A standard library implementation of .empty() might easily just use the distance between the begin and end iterator, so might end up with just as many (potentially more) calls as saved. Similarly, an optimizer might be able to optimize the calls.

In other words: unless performance measurements show differently, I'd prefer to keep the code simpler.


Repository:
  rL LLVM

https://reviews.llvm.org/D46177