[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:38
+  // call 'prettyPrintRegisterOp'.
+  DIDumpOptions DumpOpts;
+
----------------
probinson wrote:
> This should be a local variable in the place where prettyPrintRegisterOp is called; it doesn't need to be a class member.
Changed `DIDumpOptions DumpOpts;` to a local variable.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:43
+  bool RangesDataAvailable = false;
+  DWARFDie DummyDie;
+  LVAddress CUBaseAddress = 0;
----------------
probinson wrote:
> DummyDie is a placeholder used in only two places. It should be a local variable in those places, not a class member.
Changed `DWARFDie DummyDie;` to a local variable.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:512
+  // equal or greater that the line address and less than the address of the
+  // next line record.
+  LLVM_DEBUG({
----------------
probinson wrote:
> probinson wrote:
> > 
> Have you thought about using `std::merge` to do this merging of two sorted containers?  If that doesn't work, maybe refactor the three nested loops modeled on the example merge code sketched out [[ https://en.cppreference.com/w/cpp/algorithm/merge | here ]]
Changed.


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

https://reviews.llvm.org/D125783