[PATCH] D56063: [llvm-nm] Allow --size-sort to print symbols with only Symbol size

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
Comment at: test/tools/llvm-nm/X86/size-sort.test:1
+# RUN: llvm-nm --size-sort %p/Inputs/hello.obj.elf-x86_64 | FileCheck --check-prefix=SIZE_SORT_NO_ADDR %s
+# RUN: llvm-nm --size-sort -S %p/Inputs/hello.obj.elf-x86_64 | FileCheck --check-prefix=SIZE_SORT_PRINT_ADDR %s
----------------
nit: using - instead of _ in check-prefix is more common (i.e. SIZE-SORT-NO-ADDR), ditto for the rest


================
Comment at: test/tools/llvm-nm/X86/size-sort.test:5
+
+# SIZE_SORT_NO_ADDR:                  U puts
+# SIZE_SORT_NO_ADDR: 0000000000000015 T main
----------------
Looks like GNU nm will filter out undefined symbols when --size-sort is used. It isn't documented, but seems intentional from the code.
I guess we don't need to emulate that though, just making a note of this.


================
Comment at: tools/llvm-nm/llvm-nm.cpp:832
         strcpy(SymbolAddrStr, printBlanks);
-      else
+      else if(PrintAddress)
         format(printFormat, I->Address)
----------------
h4xr wrote:
> rupprecht wrote:
> > Reverting this part of the change doesn't seem to cause any tests to fail, can you add a test case that covers it?
> Hi, I have improved the test cases to account for covering the output when PrintAddress is also supplied. Please let me know, if that is the expected test case here, I will be more than happy to improve the test scenario if I understood it wrong :)
The added test cases are good, but if I change `else if(PrintAddress)` back to just `else`, then tests still pass -- meaning there isn't any regression testing for this bit.
Ideally, there should be test coverage that captures what the change here is for, otherwise it will accidentally be reverted at some point.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56063