[PATCH] D96602: [llvm-nm][test] Add additional test coverage for llvm-nm options

[Xing GUO via Phabricator via llvm-commits]

Higuoxing added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/reverse-sort.test:4-5
+# RUN: llvm-nm %t.o --reverse-sort | FileCheck %s --check-prefix=SORTED
+# RUN: llvm-nm %t.o --reverse-sort --numeric-sort | FileCheck %s --check-prefix=SORTED
+# RUN: llvm-nm %t.o --reverse-sort --size-sort | FileCheck %s --check-prefix=SORTED
+# RUN: llvm-nm %t.o --reverse-sort --no-sort | FileCheck %s --check-prefix=UNSORTED
----------------
jhenderson wrote:
> Higuoxing wrote:
> > I think it might be good to assign different values to symbol size and value. e.g.,
> > 
> > ```
> >   - Name:    second
> >     Section: .text
> >     Value:   2
> >     Size:    3
> >   - Name:    third
> >     Section: .text
> >     Value:   3
> >     Size:    1
> >   - Name:    first
> >     Section: .text
> >     Value:   1
> >     Size:    2
> > ```
> > 
> > to show that symbols are sorted by their address or size when --numeric-sort or --size-sort option is applied.
> It was a deliberate choice to keep the order the same regardless of sorting mode, as there are other tests that show that numeric-sort/size-sort/default sort all work as expected. Changing them so that the sorting order is different for each mode would complicate the test.
> 
> The current implementation is pretty clear, in that the reversing is done on the sorted array of symbols. I think for this to change, without breaking at least some part of this test in its current form would be quite difficult.
> 
> What do you think?
Thanks for the explanation. I think you're right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96602