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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added a comment.

@dblaikie: Thanks for your valuable feedback.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:186
+//   ...
+#define CREATE_OBJECT(KIND)                                                    \
+  LV##KIND *create##KIND() {                                                   \
----------------
dblaikie wrote:
> 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.
Added `undef` for all the macros.
Added the `LV` prefix which is the one used across the tool.

Would you be OK if we leave the macros for the time being and find a more elegant solution based on your suggestion (type map and function templates) for a later patch (improvement).


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:240
+  // operations creation.
+  CREATE_OPERATION(Operation, Opcode, Operand1, Operand2)
+
----------------
dblaikie wrote:
> This macro's only used once, could it be removed (for now/until it's needed)?
I introduced the macro, just for consistency with the other macros.
But, may be I am misunderstanding your point.
Do you want to remove the macro and instead use the `createLocation()` function?
Something like:
```
LVOperation *createOperation(LVSmall OpCode, LVUnsigned Operand1, LVUnsigned Operand2)
    return new (AllocatedOperation.Allocate()) LVOperation(OpCode, Operand1, Operand2);
}
```


================
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);
----------------
dblaikie wrote:
> This seems unnecessary - could be sunk into the uses.
Good point. Removed unnecessary definitions.


================
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();
----------------
dblaikie wrote:
> 
Changed to use `auto`.


================
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));
----------------
dblaikie wrote:
> 
Removed `SecondMap`.


================
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())
----------------
dblaikie wrote:
> 
Added `LVSecondMapType *`.


================
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);
----------------
dblaikie wrote:
> 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`
Good point. Replaced to use `reference` for both `Traverse` and `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());
----------------
dblaikie wrote:
> 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 `{}`)
Good observation. Changed to `else-if`. Removed extra `{}`.


================
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();
----------------
dblaikie wrote:
> 
Changed to use `auto`.


================
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));
----------------
dblaikie wrote:
> 
Changed to `reference`.


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

https://reviews.llvm.org/D137933