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

[Greg Clayton via llvm-commits]

> On Jan 5, 2017, at 9:20 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> 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.

Will do.

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

I'll not include the the NULL DIEs when iterating and just have the dumper not use the iterators.

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

I was able to fix this by overriding the -> operator so that it doesn't rely on the operator* so I was able to fix this.
> 
> 
>  
> 
> 
> ================
> 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.

OK, I already moved it into its own function in the latest diff.

I will update the inline stuff, and remove the NULL remove the iterators and repost a diff.


>  
> 
> 
> https://reviews.llvm.org/D28303