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

Tue Feb 1 07:28:45 PST 2022

oontvoo added a comment.

In D116787#3286763 <https://reviews.llvm.org/D116787#3286763>, @jhenderson wrote:

> 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.

no worries!

> 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.

Ah, right. This sorts by both name AND type. (type first, then name amongst the group of symbols of the same type).

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

Regarding (1): it's not entirely true that this only sorts by type.(as mentioned, it sorts by both type and name).  The end goal here (for me) is to have a way to deterministically sort all the symbols.  The reason I didn't go with sorting them simply by name was because maskray@ raised concerns earlier that it didn't make sense semantically (with which I agreed).

Regarding (2), how would multiple sorting types interact? eg., people specifying both `--sort-symbols=name --sort-symbols=type`. Does the order of the flag determine which one is the first sorting priority?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787