[PATCH] D120913: [NFC][llvm-nm] create a new helper function exportSymbolNamesFromFiles for --export-symbols

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2240
 
-static void dumpSymbolNamesFromFile(std::string &Filename,
-                                    std::vector<NMSymbol> *SymbolList) {
+static std::vector<NMSymbol> dumpSymbolNamesFromFile(std::string &Filename) {
+  std::vector<NMSymbol> SymbolList;
----------------
Can `Filename` be `const` (potentially by modifying other signatures too)? If so, presumably it can be a `StringRef`?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2270
+  std::vector<NMSymbol> SymbolList;
+  for (auto &FileName : InputFilenames) {
+    const std::vector<NMSymbol> &FileSymList =
----------------
If you change `dumpSymbolNamesFromFile`, the type here should change too to match.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2271-2272
+  for (auto &FileName : InputFilenames) {
+    const std::vector<NMSymbol> &FileSymList =
+        dumpSymbolNamesFromFile(const_cast<std::string &>(FileName));
+    SymbolList.insert(SymbolList.end(), FileSymList.begin(), FileSymList.end());
----------------
This `const_cast` suggests the function signature is probably wrong. If it isn't, and `Filename` needs to be mutable in that function, I'd make a local copy of the name and pass that in instead.

Unrelated, but `FileSymList` is now a reference to a local variable in the calling function, which will likely cause strange behaviour, including crashes. It shouldn't have the `const &` bit in the type signature.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2278
+  SymbolList.erase(
+      std::remove_if(SymbolList.begin(), SymbolList.end(),
+                     [](const NMSymbol &s) { return !s.shouldPrint(); }),
----------------
Use `llvm::remove_if(SymbolList, ...)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120913/new/

https://reviews.llvm.org/D120913