[PATCH] D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function

[Alex Telishev via Phabricator via llvm-commits]

alex.telishev added inline comments.


================
Comment at: lld/COFF/ICF.cpp:173-174
       if (auto *D2 = dyn_cast<DefinedRegular>(B2))
-        return D1->getValue() == D2->getValue() &&
-               D1->getChunk()->Class[Cnt % 2] == D2->getChunk()->Class[Cnt % 2];
-    return false;
+        return   std::make_tuple (D1->getValue(), D1->getChunk()->Class[Cnt % 2])
+               < std::make_tuple (D2->getValue(), D2->getChunk()->Class[Cnt % 2]);
+    return B1 < B2;
----------------
ruiu wrote:
> I don't see a need to create tuples. I believe you could do
> 
>   return D1->getValue() < D2->getValue ||
>          D1->getChunk()->Class[Cnt % 2]) < D2->getChunk()->Class[Cnt % 2];
This is not correct - the right way to compare is 

```return     (D1->getValue() < D2->getVale ())
             || ((D1->getValue() == D2->getValue ())
                  &&  (D1->getChunk()->Class[Cnt % 2]) < D2->getChunk()->Class[Cnt % 2]));```

It's very easy to make this mistake. Because of it I always write comparisons using `std::tie` or `std::make_tuple`, but if it doesn't fit the code style, I'll rewrite it.




================
Comment at: lld/COFF/ICF.cpp:184
   // Compare relocations.
-  auto Eq = [&](const coff_relocation &R1, const coff_relocation &R2) {
+  auto Less = [this](const SectionChunk *A, const coff_relocation &R1, const SectionChunk *B, const coff_relocation &R2) {
     SymbolBody *B1 = A->File->getSymbolBody(R1.SymbolTableIndex);
----------------
ruiu wrote:
> You can capture `A` and `B`, then you don't need to pass `A` and `B` to your lexicographical_compare_helper, no? Then, you can use std::lexicographical_compare instead of your own function.
No, you can't capture `A` and `B` here, because you don't know which `SectionChunk` corresponds to left and right  parts of `Less` function (if you look into source of `std::lexicographical_compare`, Comp function is called twice with different parameter order). Initially I did it the way you propose and got a lot of segfaults.


https://reviews.llvm.org/D36963