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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/globalize.test:18-23
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x2000
+    AddressAlign:    0x0000000000000010
+    Content:         "0000000000000000"
----------------
You might as well get rid of this section, since you have no symbols in it.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:329-332
+      if (!Config.SymbolsToGlobalize.empty() &&
+        std::find(std::begin(Config.SymbolsToGlobalize),
+                  std::end(Config.SymbolsToGlobalize),
+                  Sym.Name) != std::end(Config.SymbolsToGlobalize))
----------------
Two points regarding this and the SymbolsToLocalize bit above, which I only just noticed:

1) The .empty() query is unnecessary, since std::begin(<empty vector>) == std::end(<empty vector>), so find in this situation will always return as not found.
2) Could we factor out the std::find != std::end common code between this and the localize variation into something like a lambda or helper called contains/inVector or similar? (It might be worth checking that something like that doesn't already exists).


Repository:
  rL LLVM

https://reviews.llvm.org/D46177