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

Carlos Alberto Enciso via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 16 00:21:17 PST 2023


CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:186
+//   ...
+#define CREATE_OBJECT(KIND)                                                    \
+  LV##KIND *create##KIND() {                                                   \
----------------
dblaikie wrote:
> 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.
I will include the following item for a separate patch:
- Replace the macros used for the memory management with your suggested approach (map with typeinfo).


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:240
+  // operations creation.
+  CREATE_OPERATION(Operation, Opcode, Operand1, Operand2)
+
----------------
dblaikie wrote:
> 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?
Removed the macro `LV_CREATE_OPERATION`.
Use the explicit function:

```
  // Operations creation.
  LVOperation *createOperation(LVSmall OpCode, LVUnsigned Operand1,
                               LVUnsigned Operand2) {
    return new (AllocatedOperation.Allocate())
        LVOperation(OpCode, Operand1, Operand2);
  }

```


================
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:
> 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.
Thanks.


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

https://reviews.llvm.org/D137933



More information about the llvm-commits mailing list