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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

There seems to be quite a lot of new code here, but only one minor test change. The MCDisassembler ordering stuff looks like a prime candidate for gtest unit-testing. You could probably fairly easily unit-test the SymbolInfoTy comparison method too.



================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:81
+/// The symbols in the same section are sorted in ascending order.
+/// llvm-objdump -D will choose high priority symbol to display when there are
+/// symbols with the same address.
----------------
the highest priority symbol


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:85
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
+  // Compare Label first. If the symbol is not a label, it is lower than label
+  // symbol.
----------------
I think the following is a little clearer: "Label symbols have higher priorty than non-label symbols."


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:90-91
+
+  // If symbol does not have a StorageMappingClass, it is lower priority than a
+  // symbol which has a StorageMappingClass.
+  if (!StorageMappingClass)
----------------
Now that I have a better grasp of what is going on, I'd actually recommend a slightly different wording:

"Symbols with a StorageMappingClass have higher priority than those without."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387