[PATCH] D125778: [llvm-debuginfo-analyzer] 03 - Logical elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:88-99
+  LVAutoTypes *Types = nullptr;
+  LVAutoSymbols *Symbols = nullptr;
+  LVAutoScopes *Scopes = nullptr;
+  LVAutoLines *Lines = nullptr;
+
+  // Vector of elements (types, scopes and symbols).
+  // It is the union of (*Types, *Symbols and *Scopes) to be used for
----------------
dblaikie wrote:
> CarlosAlbertoEnciso wrote:
> > CarlosAlbertoEnciso wrote:
> > > psamolysov wrote:
> > > > Just a question. The `LVScope` class is not copyable at all. Why values of these `LVAutoTypes`, `LVAutoSymbols` etc. cannot be class members, not pointers to the values? At least, having the members as values eliminates a lot of `if (Children) ...` checks in the implementation.
> > > > 
> > > > If they cannot be just values, maybe it makes sense to use smart pointers here (`std::unique_ptr`?) instead of create the members with plain `new` and delete then in the destructor.
> > > As the `LVScope` represents any scope, which can be a `class`, `function`, `lexical scope`, etc, in order to reduce the class size we use pointers to those `LVAutoXXXX`. For example:
> > > 
> > > ```
> > > class ClassPointer {
> > >   void *Pointer = nullptr;
> > > };
> > > class ClassVector {
> > >   SmallVector<void *,2> Vector; 
> > > };
> > > size_t ClassPointerSize = sizeof(ClassPointer);
> > > size_t ClassVectorSize = sizeof(ClassVector);
> > > ```
> > > ClassPointerSize = 8 bytes
> > > ClassVectorSize = 32 bytes
> > > 
> > Before uploading the patches we discussed the smart pointer usage across the tool but we decided to wait until all patches are accepted. After that. the next step is to remove the new/delete machinary across the tool.
> If this is the part of the thread you were referencing here https://reviews.llvm.org/D125783#inline-1320570 - I don't think this thread does an adequate job of discussing this issue to justify postponing it to after these patches have been approved or committed.
> 
> These patches should be/should have been made to LLVM coding standards before commit, not after - an internal discussion (I assume that's what you meant by "we" - some collection of folks who worked on/owned this project, but not the LLVM community at large/code owners?) isn't a justification for this sort of thing.
> 
> I'm pretty concerned about some things (partly as mentioned here: https://reviews.llvm.org/D125783#3893003 ) I'm seeing in these patches and the responses to both pre and post-commit review feedback.
@dblaikie Thanks very much for your comments and I appreciate and understand your concerns and be sure I am not trying to ignore the LLVM policies in relation to reviews and code quality.

Taking into consideration your comments, I would like to ask you:

- Do you want the tool removed so the memory management issues can be addressed and then a new patch series posted?
or
- Would you be OK with a promise to address the issues ASAP without reverting everything first?

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778