[PATCH] D27170: Abstract almost all DwarfDebug out of the classes in DIE.cpp.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Realized a few bits of code review feedback got lost in the noise somewhere. So I applied some myself & left a few here.



================
Comment at: llvm/trunk/include/llvm/CodeGen/DIE.h:622
 
+protected:
+  void setUnit(DIEUnit *U) {
----------------
This probably shouldn't be protected - there's no inheritance happening here. DIEUnit can access 'Owner' directly and probably should do that. It gives a bit of a false sense of encapsulation, I think.


================
Comment at: llvm/trunk/include/llvm/CodeGen/DIE.h:710-714
+  /// The compile unit or type unit DIE. This variable must be an instance of
+  /// DIE so that we can calculate the DIEUnit from any DIE by traversing the
+  /// parent backchain and getting the Unit DIE, and then casting itself to a
+  /// DIEUnit. This allows us to be able to find the DIEUnit for any DIE without
+  /// having to store a pointer to the DIEUnit in each DIE instance.
----------------
Comment ended up out of date?


================
Comment at: llvm/trunk/include/llvm/CodeGen/DIE.h:715-722
+  DIE Die;
+  /// The section this unit will be emitted in. This may or may not be set to
+  /// a valid section depending on the client that is emitting DWARF.
+  MCSection *Section;
+  uint64_t Offset; /// .debug_info or .debug_types absolute section offset.
+  uint32_t Length; /// The length in bytes of all of the DIEs in this unit.
+  const uint16_t Version; /// The Dwarf version number for this unit.
----------------
Are these members accessed directly?

(ah, the first two have one direct access each, that could've used an accessor - and the others not, so I made them all private and used the two accessors in the two spots that were needed)


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1100
+typedef std::unique_ptr<CompileUnit> CompileUnitStorage;
+typedef std::vector<CompileUnitStorage> CompileUnitCollection;
 /// \brief The core of the Dwarf linking logic.
----------------
dblaikie wrote:
> Alternatively, this could be a std::deque if items are added but never removed & object identity is important. (may not be better that way - just suggesting it so you, Adrian, etc can consider it)
Any thoughts on using deque here? (Adrian?)


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2724-2726
+    Die = Info.Clone;
+    if (!Die)
+      Die = Info.Clone = DIE::get(DIEAlloc, dwarf::Tag(InputDIE.getTag()));
----------------
dblaikie wrote:
> aprantl wrote:
> > this is confusing to read. No idea how to make it better though...
> Might this be simpler as:
> 
>   if (!Info.Clone)
>     Info.Clone = ...;
>   Die = Info.Clone;
Made this change in r288423


Repository:
  rL LLVM

https://reviews.llvm.org/D27170