[PATCH] D28040: Don't store the NULL DIEs in the DIE array in the DWARFUnit.

[David Blaikie via llvm-commits]

On Wed, Dec 21, 2016 at 5:04 PM Adrian Prantl via Phabricator <
reviews at reviews.llvm.org> wrote:

> aprantl added inline comments.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:359
>    if (!isValid())
> -    return;
> +    return -1U;
>    DataExtractor debug_info_data = U->getDebugInfoExtractor();
> ----------------
> Is there a better alternative to returning a magic value?
>
>
> ================
> Comment at: tools/dsymutil/DwarfLinker.cpp:1799
>    if (DIE.hasChildren())
> -    for (auto Child = DIE.getFirstChild(); Child && !Child.isNULL();
> -         Child = Child.getSibling())
> +    for (auto Child = DIE.getFirstChild(); Child; Child =
> Child.getSibling())
>        Info.Prune &= analyzeContextInfo(Child, MyIdx, CU,
> CurrentDeclContext,
> ----------------
> Might make sense to add a method that returns something like a
> SiblingIterator at some point in the future?
>
>
> ================
> Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1050
>    auto B2 = B1.getSibling();
> -  EXPECT_TRUE(B2.getSibling().isNULL());
> +  EXPECT_FALSE(B2.getSibling().isValid());
>
> ----------------
> Should we introduce something like a hasSibling() method? This almost
> sounds like it has a corrupted sibling DIE.
>

The right thing, as you note above, is to have an iterator provided by the
parent - children shouldn't/needn't know about their siblings.


>
>
> https://reviews.llvm.org/D28040
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161222/bd858e76/attachment.html>