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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks fine to me.

Change the title/commit summary to "[NFC] Refactor llvm-nm symbol sorting" (will need the additional patch mentioned inline).



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:234-235
 
-static bool compareSymbolSize(const NMSymbol &A, const NMSymbol &B) {
-  return std::make_tuple(A.Size, A.Name, A.Address) <
-         std::make_tuple(B.Size, B.Name, B.Address);
-}
+  friend bool operator<(const NMSymbol &A, const NMSymbol &B);
+  friend bool operator>(const NMSymbol &A, const NMSymbol &B);
+};
----------------
Since the `NMSymbol` class has no private members, do you need the `friend` operator declarations?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:658-660
+    llvm::stable_sort(SymbolList, std::greater<NMSymbol>());
+  else
+    llvm::stable_sort(SymbolList);
----------------
I'm okay with this switch to `stable_sort`, but if I'm not mistaken, this means that under some cases, it might be a subtle behaviour change (in a good way) that could result in people seeing "identical" symbols in different orders before and after this patch.

To keep this patch as a pure refactor, I suggest you create a separate patch that handles this behaviour change, by simply swapping sort to stable_sort. Then, you can add the `[NFC]` tag to this patch.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:249
 
+bool operator>(const NMSymbol &A, const NMSymbol &B) { return B < A; }
+} // anonymous namespace
----------------
DiggerLin wrote:
> 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
> 
> 
> 
Just noting that if @MaskRay prefers using the lambda and avoid the `operator>`, I'm okay with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119028