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

[Digger via Phabricator via llvm-commits]

DiggerLin marked an inline comment as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:83
+/// symbols have the same address.
+
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
----------------
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


================
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.
----------------
jhenderson wrote:
> not a label
thanks


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


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:77
+  return 0;
+}
+
----------------
since we only has know that  the TC0 si low priority than storage mapping class TC, TD, TE. For other storage mapping class, we do not have priority on them currently. I am not sure using switch case  to generate the priority is a good idea or not? The advantage of using a switch case is:  if we want to change the  storage mapping class priority someday, we just need to change the switch case. @hubert.reinterpretcast , @jasonliu 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387