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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin 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.
I do not think I can do it as IsLabel .

for example , if I implement as 

 if (StorageMappingClass != SymInfo.StorageMappingClass)
    return  SymInfo.StorageMappingClass.hasValue();

it will have problem. we can get the code from optional.h

template <typename T, typename U>
bool operator==(const Optional<T> &X, const Optional<U> &Y) {
  if (X && Y)
    return *X == *Y;
  return X.hasValue() == Y.hasValue();
}

template <typename T, typename U>
bool operator!=(const Optional<T> &X, const Optional<U> &Y) {
  return !(X == Y);
}

it will compare the value of storagemappingclass too ,  not only check whether it hasValue or not.

I change the code to 
```
bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
  // Label symbols have higher priority than non-label symbols.
  if (IsLabel != SymInfo.IsLabel)
    return SymInfo.IsLabel;

  // Symbols with a StorageMappingClass have higher priority than those without.
  if (StorageMappingClass.hasValue() != SymInfo.StorageMappingClass.hasValue())
    return  SymInfo.StorageMappingClass.hasValue();

  if (StorageMappingClass.hasValue()) {
    return getSMCPriority(StorageMappingClass.getValue()) <
           getSMCPriority(SymInfo.StorageMappingClass.getValue());
  }

  return false;
}
````

I do not think it is more simple than current implement, I keep the current implement, if you still want to change to above , I can change to as above code.


================
Comment at: llvm/unittests/MC/MCDisassemblerTest.cpp:44
+
+  // Test symbols compare with itself.
+  EXPECT_FALSE(SIT1 < SIT1);
----------------
jhenderson wrote:
> compare with itself. -> comparing with themselves.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78387