[PATCH] D18004: [ELF] - refactor of SymbolBody::compare()

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Symbols.cpp:115
@@ -114,1 +114,3 @@
 
+static int compareCommons(DefinedCommon* A, DefinedCommon* B) {
+  if (A->Size < B->Size)
----------------
  DeifnedCommon* A, DefiendCommon* B

->

  DeifnedCommon *A, DefiendCommon *B

================
Comment at: ELF/Symbols.cpp:117
@@ +116,3 @@
+  if (A->Size < B->Size)
+    return -compareCommons(B, A);
+  A->MaxAlignment = std::max(A->MaxAlignment, B->MaxAlignment);
----------------
I understand the intention but this seems a bit tricky. It only saves one line, so I wouldn't use a recursion.

  if (A->Size < B->Size) {
    B->MaxAlignment = std::max(A->MaxAlignment, B->MaxAlignment);
    return -1;
  }

================
Comment at: ELF/Symbols.cpp:124
@@ -116,4 +123,3 @@
 // over the Other, tie or lose, respectively.
 template <class ELFT> int SymbolBody::compare(SymbolBody *Other) {
   assert(!isLazy() && !Other->isLazy());
----------------
grimar wrote:
> btw, I think it requires a new name. It does not just compares bodies, but also set flags, so do not what its name says.
> I dont have good suggestions about name, may be resolveBody ?
Well, I don't have an idea of good name right now. It is indeed a bit weird that the function `compare` has a side effect, but I'd leave it alone for now.

================
Comment at: ELF/Symbols.cpp:149
@@ -142,3 +148,3 @@
     return -1;
-  if (!std::get<0>(L) || !std::get<1>(L) || !std::get<2>(L))
+  if (!isDefined() || isShared() || isWeak())
     return 1;
----------------
grimar wrote:
> And I think direct names are more readable than std::get<>
Agreed.


http://reviews.llvm.org/D18004