[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 8 inline comments as done.
oontvoo added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:23-31
+# TYPE-NAME-NEXT:  Symbol {
+# TYPE-NAME-NEXT:    Name: _a (19)
+# TYPE-NAME-NEXT:    Type: Section (0xE)
+# TYPE-NAME-NEXT:    Section: __data (0x3)
+# TYPE-NAME-NEXT:    RefType: UndefinedNonLazy (0x0)
+# TYPE-NAME-NEXT:    Flags [ (0x0)
+# TYPE-NAME-NEXT:    ]
----------------
jhenderson wrote:
> oontvoo wrote:
> > jhenderson wrote:
> > > Related to my above comment re. formatting, we don't need to show that the entire symbol printing is correct - that's the responsibility of a test that is testing the `--symbols` option rather than the `--sort-symbols` option. Instead, I'd just check the name and type fields. Something like this:
> > > 
> > > ```
> > > TYPE-NAME:      Name: _a
> > > TYPE-NAME-NEXT: Type: Section
> > > TYPE-NAME:      Name: _d
> > > TYPE-NAME-NEXT: Type: Section
> > > ```
> > i can drop this in the other test (the NAME-TYPE one) but this was needed here to ensure the whitespace padding was done correctly.
> Whitespace padding isn't relevant to this test: the formatting should be tested in a basic symbol printing test, not in a test that is about the `--sort-symbols` option. Besides: without the `--strict-whitespace` and `--match-full-lines` FileCheck options, this doesn't actually verify the whitespace properly.
the comment about white  was out of date (no longer needed) 


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:623
+    const llvm::SmallVector<opts::SortSymbolKeyTy> *SortKeys) {
+
+  if (SortKeys && !SortKeys->empty()) {
----------------
jhenderson wrote:
> oontvoo wrote:
> > jhenderson wrote:
> > > Nit: don't start functions with blank lines.
> > > 
> > > I don't this is the place to be adding the predicates, because much of this code will end up being duplicated when other format support is added. Instead, I recommend moving it to just after the code that creates the ObjDumper class in llvm-readobj.cpp, and use virtual functions for the predicates, using std::bind tto handle the fact that they're member functions.
> > > 
> > > If you do that, there's no need for the sort key enum. Instead, you could delay processing the command-line option until you need it to identify which predicates to add.
> > > If you do that, there's no need for the sort key enum. Instead, you could delay processing the command-line option until you need it to identify which predicates to add.
> > 
> > Implemented the virtual func parts as suggested but still keepng the enum (albeit local/static now) because it's weird to move the opts processing part outside of the `parseOptions`  function.
> > 
> > 
> > 
> Yeah, I understand the concern. The downside with keeping it separate is that you have to effectively repeat the switch/case involved in option parsing in two places, which leads to ugly warts like the need for the `assert(false)` in the second one. Up to you though.
(ack'ed - keeping this as is for now until other format started implementing the option)


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:386
+    } else {
+      error("--sort-symbols is not supported yet for this format.");
+    }
----------------
jhenderson wrote:
> 1) Error and warning messages shouldn't end with a "."
> 2) I have concerns here: this will stop dumping as soon as an unsupported file type is encountered (even in an archive), yet that is unlikely what the user wants to happen. They more likely want to continue dumping and just not sort in this case (imagine if they had mutliple different file format objects in their input). This should be at most a warning (including the input file name), although I could even see an argument for not having it at all. It also needs testing.
1. Fixed
2. If the format doesn't support --sort-symbols then the users shouldn't specify --sort-symbols.  (Keeping it as error for consistency - similarly to how it handles unknown sort-key above, which is that if it doesn't understand it, then it's an error, rather than trying to guess. This file also doesn't have precedence for "warning")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787