[PATCH] D119028: refactor the llvm-nm of symbol sorting

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:226
 
-static bool compareSymbolAddress(const NMSymbol &A, const NMSymbol &B) {
-  bool ADefined;
-  // Symbol flags have been checked in the caller.
-  if (A.Sym.getRawDataRefImpl().p) {
-    uint32_t AFlags = cantFail(A.Sym.getFlags());
-    ADefined = !(AFlags & SymbolRef::SF_Undefined);
-  } else {
-    ADefined = A.TypeChar != 'U';
-  }
-  bool BDefined;
-  // Symbol flags have been checked in the caller.
-  if (B.Sym.getRawDataRefImpl().p) {
-    uint32_t BFlags = cantFail(B.Sym.getFlags());
-    BDefined = !(BFlags & SymbolRef::SF_Undefined);
-  } else {
-    BDefined = B.TypeChar != 'U';
+  bool isSymbolDefined() const {
+    if (Sym.getRawDataRefImpl().p) {
----------------
MaskRay wrote:
> 
thanks


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:230
+      return !(Flags & SymbolRef::SF_Undefined);
+    } else
+      return TypeChar != 'U';
----------------
MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
> 
> Re-align the next line.
thanks


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:249
 
+bool operator>(const NMSymbol &A, const NMSymbol &B) { return B < A; }
+} // anonymous namespace
----------------
MaskRay wrote:
> Is `operator>` used? If used, can the use site be refactored to only use `operator<`?
yes, the operator> is used to change


```
 llvm::sort(SymbolList, [=](const NMSymbol &A, const NMSymbol &B) -> bool {
        return Cmp(B, A);
      });
```

---->



> llvm::stable_sort(SymbolList, std::greater<NMSymbol>());

there is Jame's comment as 
https://reviews.llvm.org/D112735#inline-1137095





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119028