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

[Paul Semel via Phabricator via llvm-commits]

paulsemel 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;
----------------
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 !


================
Comment at: tools/llvm-objcopy/llvm-objcopy.h:39
+template <class C, class T>
+auto contains(const C &Vector, const T &Elt)
+    -> decltype(std::end(Vector), true) {
----------------
alexshap wrote:
> it seems to me, that the name Vector is kind of misleading here - this function (as is) is more generic - maybe replace Vector with Sequence or Container ? + change Elt to smth like X (but that's just an opinion, not super important)
Agree with you, I misnamed because I was probably thinking to the use I will make of this function !


Repository:
  rL LLVM

https://reviews.llvm.org/D46177