[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:90
+  // Symbols with a StorageMappingClass have higher priority than those without.
+  if (!SymInfo.StorageMappingClass)
+    return false;
----------------
hubert.reinterpretcast wrote:
> 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.
Latest refresh did not contain new changes here?


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:83
+/// there are symbols with the same address.
+
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
----------------
Blank line still here.


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:16
+  Optional<XCOFF::StorageMappingClass> SMC_None(None);
+  Optional<XCOFF::StorageMappingClass> SMC_TC0(XCOFF::XMC_TC0);
+  Optional<XCOFF::StorageMappingClass> SMC_TC(XCOFF::XMC_TC);
----------------
There's no need to create `Optional`s instead of passing in the values that they are created from directly.


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:21
+  SymbolInfoTy SIT2(0x110000, "sym2", SMC_None, 2, false);
+  SymbolInfoTy SIT3(0x110000, "sym3", SMC_None, 3, true);
+  SymbolInfoTy SIT4(0x110000, "sym4", SMC_TC0, 4, false);
----------------
I think this symbol is too unnatural. I believe a label here with `XMC_RW` should work.


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:30
+
+  // Test symbol with a StorageMappingClass have higher priority than those
+  // without.
----------------
s/Test symbol/Test that symbols/;


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:35
+
+  // Test symbol with a TC0 StorageMappingClass has lower priority that those
+  // with other StorageMappingClass.
----------------
s/Test symbol/Test that symbols/;
s/has/have/;


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:40
+
+  // Test label symbols have higher priorty than non-label symbols.
+  EXPECT_TRUE(SIT5 < SIT3);
----------------
Please see the requested change to the definition of `SIT3`. As it is, the ordering of the tests would have suggested that the presence of a `StorageMappingClass` is more significant than the property of being a label. The current test input would not match that expectation (but I don't think such input is possible).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387