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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 9 inline comments as done.
DiggerLin added inline comments.


================
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.
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > the highest priority symbol
> > thanks
> Hi @DiggerLin. Thank you for addressing my comments, but please try to be more careful in addressing them in their entirety:
> 
> high -> highest
> 
> (if you're uncertain about something I wrote, feel free to ask for clarity!)
thanks


================
Comment at: llvm/unittests/MC/CMakeLists.txt:20
   TargetRegistry.cpp
+  XCOFFSymbolPriorityTest.cpp
   )
----------------
jhenderson wrote:
> Traditionally, test file names are derived from the file they are testing. In other words, this should probably be `MCDisassemblerTest.cpp`
thanks


================
Comment at: llvm/unittests/MC/XCOFFSymbolPriorityTest.cpp:1
+//===- SymbolSizeTest.cpp - Tests for SymbolSize.cpp ----------------------===//
+//
----------------
jhenderson wrote:
> Please fix this header.
thanks


================
Comment at: llvm/unittests/MC/XCOFFSymbolPriorityTest.cpp:25-28
+  EXPECT_LT(SIT1, SIT2);
+  EXPECT_LT(SIT2, SIT4);
+  EXPECT_LT(SIT4, SIT5);
+  EXPECT_LT(SIT5, SIT3);
----------------
jhenderson wrote:
> It would probably be a good idea to show that these comparisons are stable - in other words, comparing them the other way around still works as expected. Consequently, I'd expect an `EXPECT_GT(SIT2, SIT1);` line or equivalent for each of these pairs.
> 
> It might also be helpful to add a comment for each test case (or pair of test cases once you add `EXPECT_GT`) explaining what each case shows, along the lines of "non-label symbols have lower priority than label symbols" etc.
thanks.

since we do not provide a XCOFFSymbolInfo::operator< , testing  EXPECT_GT will has a compile error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387