[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 added inline comments.


================
Comment at: llvm/tools/llvm-readobj/MachODumper.cpp:623
+    const llvm::SmallVector<opts::SortSymbolKeyTy> *SortKeys) {
+
+  if (SortKeys && !SortKeys->empty()) {
----------------
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.





================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:192-198
   // Flush the standard output to print the warning at a
   // proper place.
   fouts().flush();
   handleAllErrors(
       createFileError(Input, std::move(Err)), [&](const ErrorInfoBase &EI) {
         WithColor::warning(errs(), ToolName) << EI.message() << "\n";
       });
----------------
jhenderson wrote:
> If sure about the warning, I recommend refactoring this to use the new `warn` function.
(removed)


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:278-279
+      } else {
+        warn("ignored empty sort key specified  in --sort-symbols='" +
+             SortKeysString + "' at position " + Twine(pos));
+      }
----------------
jhenderson wrote:
> I think it would be simpler if this were an error. What's the motivation for a warning?
(made it an error too)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787