[PATCH] D158004: llvm-nm ignore the Import symbol file for the --export-symbol option.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/AIXLinkerImportFile.h:23-28
+// The class AIXLinkerImportFile, which is not a symbolic file at the moment.
+// The command 'llvm-nm -export-symbols' exports an empty symbol for AIX linker
+// import file that depends on the AIXLinkerImportFile. Furthermore, if we
+// decide to get symbols from AIX linker import file and derive
+// AIXLinkerImportFile from the SymbolicFile class, the llvm-nm tool will also
+// need to be updated accordingly.
----------------
I don't really follow this comment. What are you trying to explain here?

Aside: I'd delete the blank line before and after it.


================
Comment at: llvm/lib/Object/AIXLinkerImportFile.cpp:19
+
+AIXLinkerImportFile::~AIXLinkerImportFile() = default;
+Expected<std::unique_ptr<AIXLinkerImportFile>>
----------------
Please add a blank line between method defninitions.

That being said, why do you need an explicit destructor, rather than just not having the function present?


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:55
 
+## Test ignoring the parsing an imported symbol file that  begins with #!
+# RUN: echo "#!" > imp.txt
----------------
Looks like this comment has regressed. Please restore it to the suggestion I made once already.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:56
+## Test ignoring the parsing an imported symbol file that  begins with #!
+# RUN: echo "#!" > imp.txt
+# RUN: llvm-nm --export-symbols imp.txt 2>&1 | FileCheck --allow-empty --implicit-check-not={{.}} %s
----------------
You're not in a dedicated directory, so temporary files like "imp.txt" need to be based on %t somehow, e.g. `%t.imp` or `%timport.txt`


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:57
+# RUN: echo "#!" > imp.txt
+# RUN: llvm-nm --export-symbols imp.txt 2>&1 | FileCheck --allow-empty --implicit-check-not={{.}} %s
+
----------------
This is a better way of checking that the output is empty.

This test case is good, but I think you should also have one where the file contains one or more symbols. The reason for this is that in the future, if you added any form of symbolic file support to AIXImportFile, you'll want to show that the listed imports don't appear as exports, which this suggested test will cover upfront.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158004