[PATCH] D65242: [ELF] More dynamic relocation packing

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Thank you for updating the comments. That's much easier to understand now!



================
Comment at: lld/ELF/SyntheticSections.cpp:1693
+  // enables us to group by r_addend as well.
+  llvm::sort(nonRelatives, [](const Elf_Rela &a, const Elf_Rela &b) {
+    if (a.r_info != b.r_info)
----------------
I believe you need to use `llvm::stable_sort` for build reproducibility.


================
Comment at: lld/ELF/SyntheticSections.cpp:1696-1698
+    else if (config->isRela)
+      return a.r_addend < b.r_addend;
+    else
----------------
nit: no `else` after `return`.


================
Comment at: lld/ELF/SyntheticSections.cpp:1718-1721
+    do {
+      group.push_back(*i++);
+    } while (i != e && (i - 1)->r_info == i->r_info &&
+             (!config->isRela || (i - 1)->r_addend == i->r_addend));
----------------
Maybe it's little bit easier to read if you use two iterators like this?

  auto j = i + 1;
  while (i->r_info == j->r_info && (!config->isRela || i->r_addend == j->r_addend)
    j++;
  if (j - i < 3)
    ...


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

https://reviews.llvm.org/D65242