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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96602