[PATCH] D26537: Fix llvm-symbolizer to correctly sort a symbol array and calculate symbol sizes

[Kuba Brecka via llvm-commits]

kubabrecka added inline comments.


================
Comment at: unittests/Object/SymbolSizeTest.cpp:17-25
+  std::vector<SymEntry> Syms;
+  auto it = symbol_iterator(SymbolRef());
+  Syms.push_back(SymEntry{it, 0xffffffff00000000ull, 1, 0});
+  Syms.push_back(SymEntry{it, 0x00ffffff00000000ull, 2, 0});
+  Syms.push_back(SymEntry{it, 0x00ffffff000000ffull, 3, 0});
+  Syms.push_back(SymEntry{it, 0x0000000100000000ull, 4, 0});
+  Syms.push_back(SymEntry{it, 0x00000000000000ffull, 5, 0});
----------------
dberris wrote:
> This looks like you can use an initialiser list instead:
> 
> ```
> auto it = symbol_iterator(SymbolRef());
> std::vector<SymEntry> Syms{
>   SymEntry{it, ...},
>   SymEntry{it, ...}};
> ```
Right.


================
Comment at: unittests/Object/SymbolSizeTest.cpp:29-31
+  for (unsigned I = 0, N = Syms.size(); I < N - 1; ++I) {
+    EXPECT_LE(Syms[I].Address, Syms[I + 1].Address);
+  }
----------------
dberris wrote:
> How about using something like `std::is_sorted(...)`?
The test is trying to make sure the sorting predicate is correct.  Using is_sorted with the same predicate that sorted the array will not test that.


================
Comment at: unittests/Object/SymbolSizeTest.cpp:33
+
+  EXPECT_EQ(1, 1);
+}
----------------
dberris wrote:
> Do you even need this?
Right, this shouldn't be here.


https://reviews.llvm.org/D26537