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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Some nitpicky comments.



================
Comment at: include/llvm/CodeGen/DIE.h:705
 //===--------------------------------------------------------------------===//
+/// DIEUnit - Represents a compile or type unit.
+//
----------------
Please don't repeat the class name in a doxygen comment.


================
Comment at: include/llvm/CodeGen/DIE.h:706
+/// DIEUnit - Represents a compile or type unit.
+//
+class DIEUnit {
----------------
I think this regular comment in-between may break doxygen?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:124
+  const DIEUnit *U = getUnit();
+  if (U)
+    return U->getDebugSectionOffset() + getOffset();
----------------
We typically write this as:
` if (const auto *U = getUnit())`



================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:126
+    return U->getDebugSectionOffset() + getOffset();
+  llvm_unreachable("DIE must belong to a DIEUnit to find the absolute offset.");
 }
----------------
alternatively:
```
assert(getUnit() && "DIE must belong to a DIEUnit to find the absolute offset.")
return getUnit()->getDebugSectionOffset() + getOffset();
```


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:140
 
+const DIEUnit *DIE::getUnit() const {
+  const DIEUnit *Unit = getUnitOrNull();
----------------
Why not return a DIEUnit & instead? This would simplify all the call sites.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:148
+  const DIE *UnitDie = getUnitDieOrNull();
+  if (UnitDie)
+    return UnitDie->Owner.dyn_cast<DIEUnit*>();
----------------
Same comment(s) apply here :-)


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:214
+  assert(UnitTag == dwarf::DW_TAG_compile_unit ||
+         UnitTag == dwarf::DW_TAG_type_unit);
+}
----------------
) && "message")?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:217
+
+void DIEUnit::setSection(MCSection *Section) {
+  assert(!this->Section);
----------------
perhaps move these trivial accessors into the class definition?


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2724
+  } else {
+    Die = Info.Clone;
+    if (!Die)
----------------
this is confusing to read. No idea how to make it better though...


https://reviews.llvm.org/D27170