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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

> The AllocatedObjects was introduced to record their smart pointers, to minimize the changes across the tool.

This is probably the core of what I'm concerned about & the reason for more up-front review. If the logical ownership is for each node to own its children, then that's what I'd probably want to see here, rather than introducing a side table ownership "bucket".

It'd be good if this change didn't move ownership around - but mapped the ownership directly from the C code, where it's not too convoluted. Or if there's good reason (other than code churn, like "even if we were continuing to use raw new/delete, we would still want to change the ownership in this way") to change the ownership models at the same time I think more details about the old ownership and new ownership and why the new one is better would be suitable to have in the patch description.

As it stands, trees owning their children seems fine (though possibly slow to cleanup but the currently proposed change in ownership doesn't look like it'd speed that up much, and would increase memory usage I think - if perf is a problem, it might be worth considering one of LLVM's BumpPtrAllocator - if nodes are only ever allocated in one pass, never manipulated/created/destroyed later - maybe that's the good/correct and simpler path (not correct because it's simpler, but independently)?)



================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:490
   EXPECT_EQ(IterRange->first, Function->getOffset());
-  LVLocations *Locations = IterRange->second;
+  LVLocations *Locations = &IterRange->second;
   EXPECT_NE(Locations, nullptr);
----------------
Perhaps as follow-up, this could/should be changed to a reference variable, then the next line (checking non-null) would be more obviously unneeded for instance.

(similarly in the several other cases above/below)


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

https://reviews.llvm.org/D137933