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

[grimar]

Summary

That makes it a bit shorter.

[grimar]

ELF/Symbols.cpp

119 ↗

(On Diff #50163)

btw we anyways don't need that as ThisC->MaxAlignment/OtherC->MaxAlignment are of a type uint64

124 ↗

(On Diff #50163)

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 ?

149 ↗

(On Diff #50163)

And I think direct names are more readable than std::get<>

[ruiu]

ELF/Symbols.cpp

115 ↗

(On Diff #50163)

DeifnedCommon* A, DefiendCommon* B

->

DeifnedCommon *A, DefiendCommon *B

117 ↗

(On Diff #50163)

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;
}

124 ↗

(On Diff #50163)

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.

149 ↗

(On Diff #50163)

Agreed.

[grimar]

Addressed Rui's review comments.

ELF/Symbols.cpp

115 ↗

(On Diff #50163)

Done.

117 ↗

(On Diff #50163)

Done.

[grimar]

ELF/Symbols.cpp

159 ↗

(On Diff #50249)

Actually I don't like that part of code. There is to much attention to commons. But I dont have ideas how to improve that. Also in my opinion it is still better than was before.

[• rafael]

ELF/Symbols.cpp

117 ↗

(On Diff #50249)

I think you can just say

A->MaxAlingment = B->MaxAlignment = std::max(A->MaxAlignment, B->MaxAlignment);

before the if.

[grimar]

ELF/Symbols.cpp

117 ↗

(On Diff #50249)

Sounds good for me.

[ruiu]

LGTM

ELF/Symbols.cpp

117 ↗

(On Diff #50249)

Not directly related (so please don't do that in this patch), but I'd rename MaxAlignment to Align. (We can argue of the maximum alignment for a group of symbols, but for each symbol, there is only one alignment.)

lld/trunk/ELF/Symbols.cpp

 static uint8_t getMinVisibility(uint8_t VA, uint8_t VB) {
   if (VA == STV_DEFAULT)
     return VB;
   if (VB == STV_DEFAULT)
     return VA;
   return std::min(VA, VB);
 }
 
+static int compareCommons(DefinedCommon *A, DefinedCommon *B) {
+  A->MaxAlignment = B->MaxAlignment =
+      std::max(A->MaxAlignment, B->MaxAlignment);
+  if (A->Size < B->Size)
+    return -1;
+  return 1;
+}
+
 // Returns 1, 0 or -1 if this symbol should take precedence
 // over the Other, tie or lose, respectively.
 template <class ELFT> int SymbolBody::compare(SymbolBody *Other) {
-  typedef typename ELFFile<ELFT>::uintX_t uintX_t;
   assert(!isLazy() && !Other->isLazy());
   std::tuple<bool, bool, bool> L(isDefined(), !isShared(), !isWeak());
   std::tuple<bool, bool, bool> R(Other->isDefined(), !Other->isShared(),
                                  !Other->isWeak());
 
   // Normalize
   if (L > R)
     return -Other->compare<ELFT>(this);
 
   Visibility = Other->Visibility =
       getMinVisibility(Visibility, Other->Visibility);
 
   if (IsUsedInRegularObj || Other->IsUsedInRegularObj)
     IsUsedInRegularObj = Other->IsUsedInRegularObj = true;
 
   // We want to export all symbols that exist both in the executable
   // and in DSOs, so that the symbols in the executable can interrupt
   // symbols in the DSO at runtime.
   if (isShared() != Other->isShared())
     if (isa<DefinedRegular<ELFT>>(isShared() ? Other : this))
       MustBeInDynSym = Other->MustBeInDynSym = true;
 
   if (L != R)
     return -1;
-  if (!std::get<0>(L) || !std::get<1>(L) || !std::get<2>(L))
-    return 1;
-  if (isCommon()) {
-    if (!Other->isCommon())
-      return -1;
-    auto *ThisC = cast<DefinedCommon>(this);
-    auto *OtherC = cast<DefinedCommon>(Other);
-    uintX_t Align = std::max(ThisC->MaxAlignment, OtherC->MaxAlignment);
-    if (ThisC->Size >= OtherC->Size) {
-      ThisC->MaxAlignment = Align;
-      return 1;
-    }
-    OtherC->MaxAlignment = Align;
-    return -1;
-  }
-  if (Other->isCommon())
+  if (!isDefined() || isShared() || isWeak())
     return 1;
+  if (!isCommon() && !Other->isCommon())
     return 0;
+  if (isCommon() && Other->isCommon())
+    return compareCommons(cast<DefinedCommon>(this),
+                          cast<DefinedCommon>(Other));
+  return isCommon() ? -1 : 1;
 }
 
 Defined::Defined(Kind K, StringRef Name, bool IsWeak, uint8_t Visibility,
                  uint8_t Type)
     : SymbolBody(K, Name, IsWeak, Visibility, Type) {}
 
 DefinedBitcode::DefinedBitcode(StringRef Name, bool IsWeak, uint8_t Visibility)
     : Defined(DefinedBitcodeKind, Name, IsWeak, Visibility, 0 /* Type */) {}