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

[Saurabh Badhwar via Phabricator via llvm-commits]

h4xr marked 4 inline comments as done and an inline comment as not done.
h4xr 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
----------------
rupprecht wrote:
> nit: using - instead of _ in check-prefix is more common (i.e. SIZE-SORT-NO-ADDR), ditto for the rest
Have addressed this change in the latest revision


================
Comment at: tools/llvm-nm/llvm-nm.cpp:832
         strcpy(SymbolAddrStr, printBlanks);
-      else
+      else if(PrintAddress)
         format(printFormat, I->Address)
----------------
rupprecht wrote:
> 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.
So, I just took a look at the code flow once again to see why the tests weren't failing. On a more indepth look, it seems like the printing of the address and size is controlled by the logic starting at line #855 onwards.
As a matter of which, I have reverted the else-if back to the original else statement as it was there before


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56063