[PATCH] D27170: Abstract almost all DwarfDebug out of the classes in DIE.cpp.
Adrian Prantl via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 30 14:07:18 PST 2016
> 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
>
>
>
More information about the llvm-commits
mailing list