[PATCH] D125783: [llvm-dva] 08 - ELF Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:489
+  void addPublicName(LVScope *Scope, LVAddress LowPC, LVAddress HighPC) {
+    PublicNames.insert(
+        std::make_pair(Scope, LVNameInfo(LowPC, HighPC - LowPC)));
----------------
psamolysov wrote:
> These pairs: `Scope/LVNameInfo` and `LVNameInfo (LVAddress/uint64_t)` are relatively small but it could be more effective do not copy at least the first pair: `PublicNames.emplace(Scope, LVNameInfo(LowPC, HightPC - LowPC));` or do not copy all the pairs: 
> 
> ```
> PublicNames.emplace(std::piecewise_construct,
>   std::forward_as_tuple(Scope),
>   std::forward_as_tuple(LowPC, HighPC - LowPC));
> ```
Nice change. Thanks for the additional notes.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1438
+    LVPublicNames::const_iterator Iter;
+    for (OffsetSorted::value_type &Entry : SortedNames) {
+      Iter = Entry.second;
----------------
psamolysov wrote:
> 
Nice change.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:28-29
+  if (SymbolNames.find(SymbolName) == SymbolNames.end()) {
+    SymbolNames.insert(std::make_pair(
+        SymbolName, LVSymbolTableEntry(Function, 0, SectionIndex, false)));
+  } else {
----------------
psamolysov wrote:
> Just a recommendation, I'm not sure the proposed code is more readable or gives a large optimization but from another point of view it eliminates copying of a struct of 18-24 bytes (`LVSymbolTableEntry`).
Taking your recomendation; nice change.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:47
+  if (SymbolNames.find(SymbolName) == SymbolNames.end())
+    SymbolNames.insert(
+        std::make_pair(SymbolName, LVSymbolTableEntry(nullptr, Address,
----------------
psamolysov wrote:
> The previous comment about a `std::picewise_construct` construction could be applied here as well.
Applied your recomendation.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:541
+      // No previous references to this offset.
+      ElementTable.insert(std::make_pair(
+          Offset, LVElementEntry(CurrentElement, LVElementSet())));
----------------
psamolysov wrote:
> Just a nit, if you find your variant more readable, no problem.
Taking your nice variant.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1028
+    ElementTable.emplace(
+        std::make_pair(offset, LVElementEntry(nullptr, LVElementSet{Element})));
+  else {
----------------
psamolysov wrote:
> 
Taking your suggested variant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125783/new/

https://reviews.llvm.org/D125783