[PATCH] D137933: [llvm-debuginfo-analyzer] 08a - Memory Management

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:186
+//   ...
+#define CREATE_OBJECT(KIND)                                                    \
+  LV##KIND *create##KIND() {                                                   \
----------------
Undefine the macro after its uses - and maybe add an LLVM prefix to make this less likely to collide with other things?

Be nice to avoid the macros if possible, but nothing comes up quickly for me - probably some sort of type map could be used for the allocators and then these creation functions could be a function template instead. But not sure if we have a convenient type map already at hand.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:240
+  // operations creation.
+  CREATE_OPERATION(Operation, Opcode, Operand1, Operand2)
+
----------------
This macro's only used once, could it be removed (for now/until it's needed)?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:148
   void add(FirstKeyType FirstKey, SecondKeyType SecondKey, ValueType Value) {
     LVSecondMapType *SecondMap = nullptr;
     typename LVFirstMapType::iterator FirstIter = FirstMap.find(FirstKey);
----------------
This seems unnecessary - could be sunk into the uses.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:151-152
     if (FirstIter == FirstMap.end()) {
-      SecondMap = new LVSecondMapType();
-      FirstMap.emplace(FirstKey, SecondMap);
+      std::unique_ptr<LVSecondMapType> SecondMapSP =
+          std::make_unique<LVSecondMapType>();
+      SecondMap = SecondMapSP.get();
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:153-154
+          std::make_unique<LVSecondMapType>();
+      SecondMap = SecondMapSP.get();
+      SecondMap->emplace(SecondKey, Value);
+      FirstMap.emplace(FirstKey, std::move(SecondMapSP));
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:157
     } else {
-      SecondMap = FirstIter->second;
+      SecondMap = FirstIter->second.get();
+      if (SecondMap->find(SecondKey) == SecondMap->end())
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:688
           };
-          Traverse(Parent->Types, SortFunction);
-          Traverse(Parent->Symbols, SortFunction);
-          Traverse(Parent->Scopes, SortFunction);
-          Traverse(Parent->Ranges, compareRange);
-          Traverse(Parent->Children, SortFunction);
+          Traverse(Parent->Types.get(), SortFunction);
+          Traverse(Parent->Symbols.get(), SortFunction);
----------------
If `Traverse` always takes something non-null, it'd be nice if it took its argument by reference - then these call sites could be:
```
Traverse(*Parent->Types, SortFunction);
```
(& avoid the need for the explicit `.get()` call)

Similarly below for `SetCompareState`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:258-259
+        TypesParam->push_back(Type->getTypeAsType());
+      } else {
         if (Type->getIsKindScope()) {
+          ScopesParam->push_back(Type->getTypeAsScope());
----------------
idly: Should this be else-if?
And there seems to be some extra `{}` here than is usually used in LLVM (except where ambiguous, single line blocks should omit the `{}`)


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:370
   LVAddress FirstAddress = Address;
-  LVLines *Instructions = new LVLines();
+  std::unique_ptr<LVLines> InstructionsSP = std::make_unique<LVLines>();
+  LVLines *Instructions = InstructionsSP.get();
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:371
+  std::unique_ptr<LVLines> InstructionsSP = std::make_unique<LVLines>();
+  LVLines *Instructions = InstructionsSP.get();
+  DiscoveredLines.emplace_back(std::move(InstructionsSP));
----------------



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

https://reviews.llvm.org/D137933