[PATCH] D119028: [NFC] Refactor llvm-nm symbol comparing and split 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:655
+  if (ReverseSort)
+    llvm::sort(SymbolList, std::greater<NMSymbol>());
+  else
----------------
MaskRay wrote:
> Optional: this can be written as `llvm::sort(SymbolList, std::greater<>());` to reduce boilerplate.
thanks, change as  suggestion when landed


================
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:
> MaskRay wrote:
> > jhenderson wrote:
> > > 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.
> > OK, `operator>` is fine since there is a `std::greater<NMSymbol>()`.
> Actually, I think it'd be better to do:
> 
> ```
> llvm::sort(Symbols);
> if (ReverseSort)
>   std::reverse(Symbols.begin(), Symbols.end());
> ```
> 
> NMSymbol is not trivially copyable, so llvm::sort expands to std::sort.
> Having two instantiations (less and greater) increase the code size too much.
> std::reverse can decrease the code size.
yes, it can reduce the code a little, to use the 
But there is one sort, one reverse maybe less efficient  than one std::greater<NMSymbol>().  sort.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119028