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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: include/llvm/CodeGen/DIE.h:705
 //===--------------------------------------------------------------------===//
+/// DIEUnit - Represents a compile or type unit.
+//
----------------
aprantl wrote:
> Please don't repeat the class name in a doxygen comment.
I was trying to be consistent with the other classes in this file. I can remove it.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:140
 
+const DIEUnit *DIE::getUnit() const {
+  const DIEUnit *Unit = getUnitOrNull();
----------------
aprantl wrote:
> Why not return a DIEUnit & instead? This would simplify all the call sites.
Since there are clients that need the getUnitOrNull() already, I would like to keep this consistent with that call. Also I would like the code to not crash if asserts are not enabled. So I would prefer to leave this as a pointer.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:213-214
+  Die.setUnit(this);
+  assert(UnitTag == dwarf::DW_TAG_compile_unit ||
+         UnitTag == dwarf::DW_TAG_type_unit);
+}
----------------
Not sure what I would say? && "It must be one of these?". The assert seems pretty clear. It was also moved from another location in DwarfUnit::DwarfUnit where this assert appeared just like this.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:217
+
+void DIEUnit::setSection(MCSection *Section) {
+  assert(!this->Section);
----------------
aprantl wrote:
> perhaps move these trivial accessors into the class definition?
I generally don't like putting things with asserts in header files, but there are plenty of asserts in LLVM header files already, so I can move this.


================
Comment at: tools/dsymutil/DwarfLinker.cpp:219
+  CompileUnit(const CompileUnit &RHS) = delete;
+  CompileUnit(CompileUnit &&RHS) = delete;
 
----------------
Sounds good, will remove.


https://reviews.llvm.org/D27170