[PATCH] D78387: [AIX][XCOFF] add symbol priority for the llvm-objdump -D -symbol-description

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:32
+namespace objdump {
+bool compareXCOFFSymbolInfo(const XCOFFSymbolInfo &SymInfo1,
+                            const XCOFFSymbolInfo &SymInfo2);
----------------
It's not okay for the library header to refer to a function defined in the tool implementation. Either the function should be part of the library or the tool should define and use a custom comparator. I'm leaning towards the latter.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:78
+                                     const XCOFFSymbolInfo &SymInfo2) {
+  // Compare Label first. If the symbol is Label, it is in high priority.
+  if (SymInfo1.IsLabel != SymInfo2.IsLabel)
----------------
To understand the "priority" we need to understand whether sorting by this predicate (and the `operator <` that calls it) produces a list with the priority in ascending or in descending order. Please add a comment that indicates which. Doxygen-style interface documentation might be appropriate for the comparator that will be used by the tool.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:80
+  if (SymInfo1.IsLabel != SymInfo2.IsLabel)
+    return SymInfo1.IsLabel < SymInfo2.IsLabel;
+
----------------
Using `<` on booleans is weird, the expression here is equivalent to `SymInfo2.IsLabel`.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:82
+
+  // If symbol have not StorageMappingClass, it is in low proirity.
+  if (!SymInfo1.StorageMappingClass)
----------------
There should be additional tiebreakers until the symbols are mostly indistinguishable. The symbol with the index is higher priority than the one without. Since we already prefer labels over csects, the symbol with the lower index is likely to be higher priority (at least if both have a storage mapping class; if not, then it's hard to tell). We can retain the old final tiebreaker on the name as well.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:96
+
+  return SymInfo1.StorageMappingClass.getValue() <=
+         SymInfo1.StorageMappingClass.getValue();
----------------
We must never return `true` for comparing a symbol with itself. Also, we should prefer lower storage mapping class values: this conveniently means that the code interpretation is preferred over the data one (code being less likely to be of zero length).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387