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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 4 inline comments as done.
DiggerLin 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;
----------------
jhenderson wrote:
> Can `Filename` be `const` (potentially by modifying other signatures too)? If so, presumably it can be a `StringRef`?
thanks


================
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());
----------------
jhenderson wrote:
> 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.
according to https://en.cppreference.com/w/cpp/language/lifetime 

The lifetime of a temporary object may be extended by binding to a const lvalue reference or to an rvalue reference (since C++11), see reference initialization for details.

when 'return SymbolList;" in dumpSymbolNamesFromFile , the compiler will create a temporary object "SymbolList" , 

So it will not crash.


================
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(); }),
----------------
jhenderson wrote:
> Use `llvm::remove_if(SymbolList, ...)`
there is no api llvm::remove_if(SymbolList, ...) as https://en.cppreference.com/w/cpp/algorithm/remove
and if you use the llvm::remove_if(SymbolList, ...) it will compile error. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120913