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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly looks good



================
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) {
----------------



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

Re-align the next line.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119028