[PATCH] D116787: [llvm-readobj][MachO] Add option to sort the symbol table before dumping (MachO only, for now).

Wed Jan 12 12:41:45 PST 2022

oontvoo added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:1
+## Verify that llvm-readobj can dump files with stab symbols in a sorted order,
+
----------------
jhenderson wrote:
> I'd name this file `sort-symbols.yaml` to match the option name. Also, is it really relevant that "stabs" are mentioned in the comment? Are all symbols stabs? If not, do non-stab symbols get sorted too (note: not a Mach-O developer, so I may be talking nonsense!).
No, technically, not all symbols are STABS symbols .
 ( but non-stabs  symbols wouldn't be printed here, so ... ) I was just trying to keep the names consistent with the other file (since both of these tested that STABS symbols can be dumped correctly).

================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:54
+
+--- !mach-o
+FileHeader:
----------------
jhenderson wrote:
> As you've now got a separate YAML, I'd change your symbol names to emphasise the differences, rather than being basically unrelated cruft copied over from the old test.
Actually the names are quite realistic and  are representative enough for what I wanted to test. I'm not really seeing why they need to change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787