That makes it a bit shorter.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
149 ↗ | (On Diff #50163) | And I think direct names are more readable than std::get<> |
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. |
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. |
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. |
ELF/Symbols.cpp | ||
---|---|---|
117 ↗ | (On Diff #50249) | Sounds good for me. |
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.) |