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

Thu Feb 24 09:58:37 PST 2022

oontvoo added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:54
+
+--- !mach-o
+FileHeader:
----------------
jhenderson wrote:
> 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.
Just to be clear, when we sort by sections, we actually sort by their encoded values (numeric values, eg, 0x2E for debug ) and not the section names. So naming "_section1", "_section2", etc, doesn't help clarifying that point.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787