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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Thanks



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:186
+//   ...
+#define CREATE_OBJECT(KIND)                                                    \
+  LV##KIND *create##KIND() {                                                   \
----------------
CarlosAlbertoEnciso wrote:
> 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).
`LV` doesn't seem like a sufficiently unique prefix - I'd expect all the LLVM macros to start with `LLVM` - do they not? Hmm, a quick grep seems to indicate yeah, a bunch of LLVM macros don't use a more strict prefix. Pity about that. Ah well.

*shrug*

Sure, separate patch is OK. Probably a standard (or LLVM) map with typeinfo (would need a specialization to expose equality and hasher) as the key might suffice.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:240
+  // operations creation.
+  CREATE_OPERATION(Operation, Opcode, Operand1, Operand2)
+
----------------
CarlosAlbertoEnciso wrote:
> 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);
> }
> ```
yeah, since there's only one, that seems OK?


================
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);
----------------
CarlosAlbertoEnciso wrote:
> 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`.
Actually now it looks like traverse takes the unique_ptr by reference, rather than the underlying value by reference, which I guess is necessary if they can be null. So, sounds good.


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

https://reviews.llvm.org/D137933