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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778