[PATCH] D137933: [llvm-debuginfo-analyzer] 10 - Smart Pointers

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:79
+  using LVObjectPtr = std::unique_ptr<LVObject>;
+  SmallVector<LVObjectPtr> AllocatedObjects;
+
----------------
dblaikie wrote:
> This addition of a new owning data structure (similar to some other comments I've made) makes me a bit uncomfortable - were there existing memory ownership issues in this code that this is addressing, or was the existing ownership hard to model in C++ explicit ownership? Or some other reason this is being introduced?
> or was the existing ownership hard to model in C++ explicit ownership? Or some other reason this is being introduced?

Correct.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:431
+  LVLines *Instructions = InstructionsPtr.get();
+  DiscoveredLines.emplace_back(std::move(InstructionsPtr));
 
----------------
dblaikie wrote:
> What's the ownership model for "Instructions"/why did `DiscoveredLines` get introduced in this patch? (was there a memory ownership issue in the raw-new-using code that this is addressing? Otherwise I Wouldn't expect to see new containers in this patch)
The raw-new-using code stored those 'Instructions' in the 'LVDoubleMap` container. It was not very clear.
Now they are stored at the `Reader` scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137933