[PATCH] D18004: [ELF] - refactor of SymbolBody::compare()
Rui Ueyama via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 9 13:03:08 PST 2016
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
More information about the llvm-commits
mailing list