<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 4, 2017 at 11:43 AM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:447<br class="gmail_msg">
+ return make_range(begin(), end());<br class="gmail_msg">
+}<br class="gmail_msg">
----------------<br class="gmail_msg">
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:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
DWARFDie Die;<br class="gmail_msg"></blockquote><div><br>Fair enough - could still make them inline (but out-of-class) definitions in the header if you like. They're short/simple enough.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
```<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: tools/dsymutil/DwarfLinker.cpp:1800-1801<br class="gmail_msg">
+ for (auto Child: DIE.children()) {<br class="gmail_msg">
+ if (Child.isNULL())<br class="gmail_msg">
+ break;<br class="gmail_msg">
Info.Prune &= analyzeContextInfo(Child, MyIdx, CU, CurrentDeclContext,<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> When can a child be null? Oh, we're still leaving null as a child in the iteration. That seems really weird.<br class="gmail_msg">
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?<br class="gmail_msg"></blockquote><div><br>For my money I'd have dumping reconstitute the null, rather than having the API expose a way to visit it.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1138<br class="gmail_msg">
+ // C1<br class="gmail_msg">
+ // D<br class="gmail_msg">
+ auto CUDie = CU.getUnitDIE();<br class="gmail_msg">
----------------<br class="gmail_msg">
ok, I can simplify and remove B1, B2, C, C1 and D.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1226-1227<br class="gmail_msg">
+ end = Null.end();<br class="gmail_msg">
+ EXPECT_FALSE((*begin).isValid());<br class="gmail_msg">
+ EXPECT_FALSE((*end).isValid());<br class="gmail_msg">
+ EXPECT_EQ(begin, end);<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> can use x->y rather than (*x).y, hopefully<br class="gmail_msg">
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.</blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.<br class="gmail_msg"></blockquote><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1228<br class="gmail_msg">
+ EXPECT_FALSE((*end).isValid());<br class="gmail_msg">
+ EXPECT_EQ(begin, end);<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
No, you can't get a NULL die without creating DWARF for it, so this should probably stay here.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1236<br class="gmail_msg">
+ EXPECT_FALSE((*end).isValid());<br class="gmail_msg">
+ EXPECT_EQ(begin, end);<br class="gmail_msg">
+}<br class="gmail_msg">
----------------<br class="gmail_msg">
Why? this is testing the iterators and their functionality.<br class="gmail_msg"></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D28303" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D28303</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>