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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:50
+  using LVSymbolNames = std::map<std::string, LVSymbolTableEntry>;
+  LVSymbolNames SymbolNames;
+
----------------
Does an associative container like `StringMap` suffice?

If a sorted associative container is needed, consider std::map<StringRef, ...> if the keys are backed somewhere.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:109
+
+  std::unique_ptr<const MCRegisterInfo> MRI = nullptr;
+  std::unique_ptr<const MCAsmInfo> MAI = nullptr;
----------------
Drop `= nullptr` 


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:48
+  // Current elements during the processing of a DIE.
+  LVElement *CurrentElement = nullptr;
+  LVScope *CurrentScope = nullptr;
----------------
I haven't checked but whether a smart pointer is suitable here? You assign these fields with `new`ed objects. it is prone to a memory leak.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125783