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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/ICF.cpp:99
+
+  // I'm not sure if regular sort can be used here
+  std::stable_sort (Chunks.begin() + Begin, Chunks.begin() + End, Less);
----------------
This comment is not useful because it poses a question to a reader instead of answering a question for a reader.

You cannot actually use std::sort because we want a deterministic output in general. For a set of input files, there are a lot of valid link results for it, but we always want to output the same file, as it is important for build reproducibility.


================
Comment at: lld/COFF/ICF.cpp:102
+
+  auto curGroupBegin = Begin;
+  for (auto curPos = Begin + 1; curPos < End; curPos++)
----------------
zturner wrote:
> We need to use LLVM naming conventions here, which is that all variables are CamelCase.
auto -> size_t

Basically, you want to update the patch so that your new code looks like the existing code in this file. At the moment, your code looks different because of the usage of auto, coding style, variable naming style, parentheses, use of cast to `void`, etc.


================
Comment at: lld/COFF/ICF.cpp:158
   // Compare relocations.
-  auto Eq = [&](const coff_relocation &R1, const coff_relocation &R2) {
-    if (R1.Type != R2.Type ||
-        R1.VirtualAddress != R2.VirtualAddress) {
-      return false;
-    }
+  auto Less = [this] (const SectionChunk *A, const coff_relocation &R1, const SectionChunk *B, const coff_relocation &R2) {
+      auto to_tuple = [] (const coff_relocation &rel) {
----------------
Did you have to replace `&` with `this`?


================
Comment at: lld/COFF/ICF.cpp:163-164
+
+    if (to_tuple (R1) != to_tuple (R2))
+      return to_tuple (R1) < to_tuple (R2);
+
----------------
It seems too smart. Dumb but straightforward code would probably be more readable.

  if (R1.Type != R2.Type)
    return R1.Type < R2.Type;
  if (R1.VirtualAddress != R2.VirtualAddress)
    return R1.VirtualAddress < R2.VirtualAddress;


================
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;
----------------
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];


================
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);
----------------
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.


https://reviews.llvm.org/D36963