[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.

This behaviour seems significantly under-tested. Just looking at the new operator, there are three different comparisons, so I'd expect to see at least 4 test cases for that alone (one for each side of the branch point). Furthermore, there's a big new switch in the same file that isn't exercised in any depth.



================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:83
+/// symbols have the same address.
+
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
----------------
Nit: Get rid of the blank line after the comment.

> high order symbol
It's not clear to me what this phrase means.

> symbols have the same
Should be symbols with the same


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:85
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
+  // Compare Label first. If the symbol is not label, it is lower than label
+  // symbol.
----------------
not a label


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:90
+
+  // If symbol have not StorageMappingClass, it is lower than a symbol which has
+  // StorageMappingClass.
----------------
If a symbol does not have a StorageMappingClass ... which has a StorageMappingClass


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387