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

Tue Feb 1 02:09:08 PST 2022

jhenderson added a comment.

In D116787#3251362 <https://reviews.llvm.org/D116787#3251362>, @oontvoo wrote:

> @jhenderson : Do you have any other thought on this? Thanks! :)

Apologies for the delay, I was on paternity leave for 2 weeks, and am only now getting back to reviews.

Sorry I didn't spot this earlier: it's not obvious to me that `--sort-symbols` sorts by type. I'd expect it to sort by name, probably. I'm okay with a sort option, but I think we need to reconsider the UI, especially if you are planning on rolling out this option to other formats. There may come a point in the future that people want to sort by other fields. I don't think we need to support this right away, but we could name the switch something flexible enough. A couple of ideas:

1. Simplest: `--sort-symbols-by-type`. Just a rename of the switch you've implemented here.
2. More complicated, but perhaps better UI? `--sort-symbols=type`, allowing future extensibility i.e. the ability to add e.g. `--sort-symbols=name` or `--sort-symbols=size` at some point.

Thoughts?

================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:54
+
+--- !mach-o
+FileHeader:
----------------
oontvoo wrote:
> jhenderson wrote:
> > oontvoo wrote:
> > > 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.
> > It's more about clarity of test. By using "realistic" symbol names, you're actually making it a little harder to see what is important in the testing, as people may just assume they are cruft leftover from how the test input was generated. On the other hand, if you used names like "a", "b", "c" etc, it would be very obvious if they are/are not sorted.
> done - renamed the symbols and added a few more
I missed the bit about the sorting being done by n_type (I assumed it was based on name). Sorry for the noise, but I suggest you change the names again, to name them after the n_type field they are for, e.g. "_section1", "_section2", "_symDebugTable1" etc.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787