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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

What happened to the CommandGuide update?



================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml:9
+# RUN: llvm-readobj --syms --sort-symbols=type,name %t | FileCheck %s --strict-whitespace --match-full-lines --check-prefix TYPE-NAME
+# RUN: llvm-readobj --syms --sort-symbols=name,type %t | FileCheck %s --strict-whitespace --match-full-lines --check-prefix NAME-TYPE
+
----------------
jhenderson wrote:
> Might be worth another case where just one of these values is specified, showing what happens in this case (e.g. just type means identical type symbols are left in their original order, or something like that).
> 
> As this test case is about whether or not the symbols are sorted, and not the formatting of the output, I'd remove the `--strict-whitespace` and --match-full-lines` options.
Looks like you haven't addressed my test comments?


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


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:49
+// Comparator to compare symbols.
+// Usage: caller register predicates (ie., how to compare the symbols) by
+// calling addPredicate(). The order in which predicates are registered is also
----------------



================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:52
+// their priority.
+final class SymbolComparator {
+public:
----------------
Don't use `final`. It doesn't add any value and just makes later updates more complex.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:72
+private:
+  llvm::SmallVector<CompPredicate, 2> Predicates;
+};
----------------
We're inside the `llvm` namespace already.


================
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";
       });
----------------
If sure about the warning, I recommend refactoring this to use the new `warn` function.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:265
+    int pos = 0;
+    for (llvm::StringRef KeyStr : llvm::split(A->getValue(), ",")) {
+      if (!KeyStr.empty()) {
----------------



================
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));
+      }
----------------
I think it would be simpler if this were an error. What's the motivation for a warning?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787