<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 11, 2017 at 3:29 PM 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: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:186-187<br class="gmail_msg">
+ /// or Default if the value wasn't valid or wasn't a string value.<br class="gmail_msg">
+ inline const char*<br class="gmail_msg">
+ toString(const Optional<DWARFFormValue>& V, const char *Default) {<br class="gmail_msg">
+ if (V) {<br class="gmail_msg">
----------------<br class="gmail_msg">
Not sure if we need this one, but I wanted to add it just in case so we can see that it would work. I would probably save a few bytes on a line. Like things in the unit tests currently is:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
EXPECT_EQ(DieDG.getAttributeValueAsUnsignedConstant(Attr_DW_FORM_data1)<br class="gmail_msg">
.getValueOr(0),<br class="gmail_msg">
Data1);<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
And it would become either:<br class="gmail_msg">
```<br class="gmail_msg">
EXPECT_EQ(Data1, dwarf::ToUnsigned(DieDG.find(Attr_DW_FORM_data1)).getValueOr(0));<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
Or:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
EXPECT_EQ(Data1, dwarf::ToUnsigned(DieDG.find(Attr_DW_FORM_data1), 0));<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
I kind of like being able to specify the default.<br class="gmail_msg"></blockquote><div><br></div><div>I'm still a bit more interested in having shorter/more independent tests that fail sooner (ASSERT rather than EXPECT) rather than having later pieces of test code using things like default values to continue forward progress when a previous check failed.</div><div><br>No worries, though - carry on as works best for you, defaults, etc.</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: lib/DebugInfo/DWARF/DWARFDie.cpp:147<br class="gmail_msg">
+Optional<DWARFFormValue><br class="gmail_msg">
+DWARFDie::findRecurse(dwarf::Attribute Attr) const {<br class="gmail_msg">
+ if (!isValid())<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> probably 'findRecursively' (open to other ideas on naming 'findIndirectly' seems like it sort of describes things better - but it's not /always/ indirect... meh)<br class="gmail_msg">
I am open to ideas as well. findRecursively sound fine to me. Other ideas:<br class="gmail_msg">
- findAll<br class="gmail_msg">
- recursiveFind<br class="gmail_msg">
<br class="gmail_msg">
Or maybe we can rename "find" to "get" and the "get" function would only look in the current DIE, while "find" would go through the abs or spec dies?<br class="gmail_msg"></blockquote><div><br></div><div>I'm inclined towards 'find' because, like container's find (std::map::find) that do work. 'get' feels like it implies O(1) that probably doesn't fail, not an O(N) lookup that might fail.<br><br>findRecursively seems like it'll suffice. (findWithIndirection? meh)</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: lib/DebugInfo/DWARF/DWARFDie.cpp:157<br class="gmail_msg">
+ if (auto Value = Die.findRecurse(Attr))<br class="gmail_msg">
+ return Value;<br class="gmail_msg">
+ return None;<br class="gmail_msg">
----------------<br class="gmail_msg">
It usually will be just one that I am aware of, but I can't say for sure. I would rather be safe on this one if possible.<br class="gmail_msg"></blockquote><div><br></div><div>I'm not sure it's a matter of safety though - in the simplest case, this could cause invalid inputs to cause LLDB to stack overflow (construct an entity that refers to itself (or an indirect loop) via specification or abstract_origin) - if there's no valid input that needs to be supported that has multiple abstract_origin or specification steps, it might be better to constrain it.<br><br>If not, then it'd be good to have tests for interesting cases like multiple steps, loops, etc. (for the "take only one step" implementation, I wouldn't bother testing the self-referential case - it's not interesting there, doesn't cause new/interesting behavior, require special handling, etc)</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">
<a href="https://reviews.llvm.org/D28581" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D28581</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>