[PATCH] D28303: Add iterator support to DWARFDie to allow child DIE iteration.

[David Blaikie via llvm-commits]

On Wed, Jan 4, 2017 at 11:43 AM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> clayborg added inline comments.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:447
> +  return make_range(begin(), end());
> +}
> ----------------
> I can inline children(), but not begin() and end() since they use the
> iterator class which must be forward declared since it has a member
> variable:
>
> ```
>   DWARFDie Die;
>

Fair enough - could still make them inline (but out-of-class) definitions
in the header if you like. They're short/simple enough.


> ```
>
>
>
> ================
> Comment at: tools/dsymutil/DwarfLinker.cpp:1800-1801
> +    for (auto Child: DIE.children()) {
> +      if (Child.isNULL())
> +        break;
>        Info.Prune &= analyzeContextInfo(Child, MyIdx, CU,
> CurrentDeclContext,
> ----------------
> dblaikie wrote:
> > When can a child be null? Oh, we're still leaving null as a child in the
> iteration. That seems really weird.
> Yeah, we currently want it there for dumping, but not for real world use.
> Should we add a "bool IncludeNull" as a parameter to the children function
> to allow clients to choose?
>

For my money I'd have dumping reconstitute the null, rather than having the
API expose a way to visit it.


>
>
> ================
> Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1138
> +    //     C1
> +    //   D
> +    auto CUDie = CU.getUnitDIE();
> ----------------
> ok, I can simplify and remove B1, B2, C, C1 and D.
>
>
> ================
> Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1226-1227
> +  end = Null.end();
> +  EXPECT_FALSE((*begin).isValid());
> +  EXPECT_FALSE((*end).isValid());
> +  EXPECT_EQ(begin, end);
> ----------------
> dblaikie wrote:
> > can use x->y rather than (*x).y, hopefully
> I currently return a DWARFDie instance from the operator * for
> DWARFDie::iterator as I didn't want clients to modify the DWARFDie in the
> iterator by returning it by reference since it is what controls the
> iteration.


Perhaps you can return it by const reference? But you should return a
reference to be a compliant iterator - so that x->y works as expected, etc.


> This makes the -> operator not work as it takes the address of the
> returned DWARFDie and causes a compile error. Should we just have
> "DWARFDie::iterator operator*" return a reference to the contained item? I
> didn't like either approach.
>




>
>
> ================
> Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1228
> +  EXPECT_FALSE((*end).isValid());
> +  EXPECT_EQ(begin, end);
> +
> ----------------
> No, you can't get a NULL die without creating DWARF for it, so this should
> probably stay here.
>
>
> ================
> Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1236
> +  EXPECT_FALSE((*end).isValid());
> +  EXPECT_EQ(begin, end);
> +}
> ----------------
> Why? this is testing the iterators and their functionality.
>

Each test function should test one thing, ideally - not one broad feature,
but one interaction. That way more tests can be run in parallel, or
individually (if you get a test failure, you can run specifically the thing
that's of interest). Also makes tests easier to follow because it shows the
independence.


>
>
> https://reviews.llvm.org/D28303
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170105/14944a3f/attachment.html>