[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/lib/MC/MCDisassembler/MCDisassembler.cpp:83
+/// symbols have the same address.
+
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 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
> thanks
Nit: Blank line still here.


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:85
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
+  // Label symbols have higher priorty than non-label symbols.
+  if (IsLabel != SymInfo.IsLabel)
----------------
Nit: s/priorty/priority/;


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:90
+  // Symbols with a StorageMappingClass have higher priority than those without.
+  if (!SymInfo.StorageMappingClass)
+    return false;
----------------
Please use the same construction as used for `IsLabel` to make a decision based on (`SymInfo.`)`StorageMappingClass.hasValue()` only if there is a difference between the two.


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:25
+
+  // Test symbol with higher address has higher priority than those with lower
+  // address.
----------------
I think the description here puts too much emphasis on the "priority" result of this particular comparison when the "priority" is a concept that was meant to describe the sorting when the address is the same.

Suggestion:
"Test that higher addresses would appear later than lower ones when symbols are sorted in ascending order."

For future reference, it would be preferable if the less significant fields of the symbols differed in such a way that each field would suggest the reverse answer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387