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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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.
----------------
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!)


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


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


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387