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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked 12 inline comments as done.
oontvoo added inline comments.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:634-638
+      // Hack!
+      // Added a fake list scope so that the Symbol have the right indentation.
+      // Without it, each symbol will have zero-indent (because we print them
+      // individually)
+      ListScope Group(*Printer, "");
----------------
jhenderson wrote:
> I think there might be a cleaner way of doing this: `ScopedPrinter` has a `getIndentLevel` method, which you could use to set the indentation of the new printer you have here, based on that of the existing one. This will make this code more reusable too, since in different contexts, the indentation will be different.
> 
> It would look something like this: `Printer.indent(W.getIndentLevel());`, although you might need a "+ 1" or similar, I'm not sure.
Thanks!  That does look better! 


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:662
+
+    ListScope Group(W, "Symbols");
+    for (auto &Tup : SortedSymbols)
----------------
jhenderson wrote:
> Note that if you move this `ListScope` earlier, outside the `if`, you a) avoid the duplication with the `else`, and b), shouldn't need the `+ 1` I mentioned in my earlier comment about indentation.
I initially moved it here (closer to where it is used) for clarity. Anyways, i can move it back.



================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:258
   opts::VersionInfo = Args.hasArg(OPT_version_info);
+  opts::SortSymbols = Args.hasArg(OPT_sort_symbols);
 
----------------
jhenderson wrote:
> This option is currently Mach-O specific, so move it accordingly, unless you plan on implementing other formats. Also, these lists are in alphabetical order. Please maintain that order.
Yes, I plan to add that to at least, ELF - but dont want to do it in one patch. (esp. since the ELF-dumper is a bit more complex)
Fixed the ordering, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787