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

[Adrian Prantl via llvm-commits]

> On Nov 30, 2016, at 12:50 PM, Greg Clayton via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> 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.


What I thought was having:

const DIEUnit &getUnit() const; // asserts if !unit.
const DIEUnit *getUnitOrNull() const;

If every caller of getUnit() has to check for null anyway, why even have two separate calls?

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

How about "expected a unit TAG"?
It should probably also accept DW_TAG_partial_unit.

> 
> 
> ================
> 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
> 
> 
>