[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:1921
+static bool checkMachOAndArchFlags(SymbolicFile *O,
+                                   const std::string &Filename) {
   auto *MachO = dyn_cast<MachOObjectFile>(O);
----------------
Bonus patch, if you want: swap all these `const std::string &` for `StringRef`, especially since you're changing them anyway.


================
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());
----------------
DiggerLin wrote:
> 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.
Huh, fair enough. Is there a reason not to take it by value though? I suspect it's identical in terms of codegen.


================
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(); }),
----------------
DiggerLin wrote:
> 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. 
`llvm::remove_if` is a wrapper around `std::remove_if`: https://github.com/llvm/llvm-project/blob/eddd94c27df609113af1d1b795d8483aa6dd662c/llvm/include/llvm/ADT/STLExtras.h#L1623


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120913