[PATCH] D118193: [llvm-nm] add a new option -X to specify the type of object file llvm-nm should examine

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:58
+Symbols:
+  - Name:    [[VAR]]
+    Binding: STB_GLOBAL
----------------
As suggested in my earlier comment, you can reuse the class macro, to avoid needing an extra variable. The fact that the symbol is both "global" and "data" is irrelevant to the test.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:57
+    Type:  SHT_PROGBITS
+    Flags: [SHF_ALLOC, SHF_WRITE]
+Symbols:
----------------
jhenderson wrote:
> Flags aren't important for this test case. Delete them.
Ping. You've not deleted the flag field, despite this being marked as done. Please don't mark things as done unless you've uploaded a diff with the comment addressed.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:59-60
+Symbols:
+  - Name:    global_data_symbol32
+    Binding: STB_GLOBAL
+    Section: .data
----------------
jhenderson wrote:
> The default binding is Local. This is sufficient for this test case, so delete the explicit binding and rename the symbol.
Ping. You've not deleted the binding field, despite this being marked as done. Please don't mark things as done unless you've uploaded a diff with the comment addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193