[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/unittests/MC/MCDisassemblerTest.cpp:27
+  // address.
+  EXPECT_LT(SIT1, SIT2);
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > If EXPECT_GT doesn't work, perhaps you could test something like `EXPECT_FALSE(SIT2 < SIT1);` etc? Probably also `EXPECT_FALSE(SIT1 < SIT1)`?
> > 
> > I think it's important to demonstrate both the true cases, like you have done, and the false cases. If you don't then your less than operator function could be the below to make all the unit tests pass:
> > 
> > ```
> > bool operator< (auto LHS, auto RHS) {
> >   return true;
> > }
> > ```
> I do not think we need to test EXPECT_FALSE(SIT1 < SIT1) , if there are two symbol is the same, we do not care which symbol is display.
It's important to test this because a number of sorting-related algorithms use std::less to compare symbols. If two identical values were to compare as less to each other simultaneously, it will render the algorithm unstable and potentially non-deterministic. Non-deterministic code is always bad, because it can make it hard to reproduce things. In some cases, an unsuitable comparator may even cause assertions (I believe MSVC extra checks do checks to ensure comparators are sane).

Furthermore, you are focused on sorting, but the code you have written is more widely applicable. People may use it in the future assuming it works like they would expect. They would expect the less than operator to return false when comparing the same symbol to itself. This could result in bugs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387