<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 11, 2017, at 11:52 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Menlo-Regular; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Jan 11, 2017 at 11:46 AM Greg Clayton <<a href="mailto:clayborg@gmail.com" class="">clayborg@gmail.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><br class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Jan 11, 2017, at 11:33 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="gmail_msg m_5677711198769208105Apple-interchange-newline"><div class="gmail_msg"><div dir="ltr" class="gmail_msg" style="font-family: Menlo-Regular; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Wed, Jan 11, 2017 at 11:08 AM Greg Clayton <<a href="mailto:clayborg@gmail.com" class="gmail_msg" target="_blank">clayborg@gmail.com</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><br class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Jan 11, 2017, at 10:36 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="gmail_msg m_5677711198769208105m_1005218309610047010Apple-interchange-newline"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">Would it make sense to push the value retrieval down into DWARFFormValue to separate getting the value from a DIE from getting the value into some particular type - they seem pretty orthogonal & it's odd that DWARFDie handles the type handling side of that rather than an attribute/form abstraction.<br class="gmail_msg"><br class="gmail_msg">Perhaps it'd be a bit abusive, but we could even overload op[](dwarf::Attribute) on DWARFDie for easy map-like attribute lookup. Maybe 'find' would be a better name, though.<br class="gmail_msg"><br class="gmail_msg">I'm OK omitting the 'attribute' part of the name entirely if everyone's OK with overloading in this context & if the callsites tend to be pretty self documenting. (mostly if you're searching for a particular attribute you've probably got the attribute name in a literal at the callsite anyway, right?)<br class="gmail_msg"><br class="gmail_msg">So code like this, perhaps:<br class="gmail_msg"><br class="gmail_msg"> <span class="m_5677711198769208105Apple-converted-space gmail_msg"> </span>x = Die.find(DW_AT_name).asString();<br class="gmail_msg"><br class="gmail_msg">or perhaps:<br class="gmail_msg"><br class="gmail_msg"> <span class="m_5677711198769208105Apple-converted-space gmail_msg"> </span>x = asString(Die.find(DW_AT_name));<br class="gmail_msg"><br class="gmail_msg">(so that find/findAttr/[] whatever we call it can continue to return Optional<DWARFFormValue> which asString can pass along as NOne if it's passed None (or passed a non-string value), and otherwise decompose into a string, etc)<br class="gmail_msg"><br class="gmail_msg"></div></div></blockquote></div><div class="gmail_msg"><br class="gmail_msg"></div></div><div class="gmail_msg" style="word-wrap: break-word;">We basically have find already in:<div class="gmail_msg"><span class="gmail_msg" style="font-size: 11px;"><br class="gmail_msg"></span></div><div class="gmail_msg"><span class="gmail_msg" style="font-size: 11px;">Optional<DWARFFormValue> </span>DWARFDie::<span class="gmail_msg" style="font-size: 11px;">getAttributeValue(dwarf::Attribute);</span></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">We can easily rename it.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><div class="gmail_msg">If find returns an optional then your first code example would need to be:</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">x = Die.<span class="gmail_msg" style="font-size: 11px;">find</span>(DW_AT_name).getValueOr(DWARFFormValue()).asString();</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">The second form could be made to work b<span class="gmail_msg" style="font-size: 11px;">ut it doesn’t look as natural IMHO. </span></div><div class="gmail_msg"><span class="gmail_msg" style="font-size: 11px;"><br class="gmail_msg"></span></div><div class="gmail_msg">Because of the first is awkward and the second seems less clear than just asking the DIE for the value I do think there is value in having the calls on DWARFDie. Let me know what you think.</div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">I'd still probably go towards the second - for orthogonality of features. Means a DWARFFormValue can be passed around and its value retrieved later, etc.<br class="gmail_msg"><br class="gmail_msg">But would be good to hear from Adrian too. <br class="gmail_msg"><br class="gmail_msg">Another option that Adrian might have thoughts on, would be to remove the Optional wrapper around the DWARFFormValue from getAttributeValue. It's not something I'd be totally happy with - Optional's nice and explicit about the fact that there might not be a value, but if DWARFFormValue already has a "not present" state in it, we could arguably use that state to indicate that.</div></div></div></div></blockquote></div><br class="gmail_msg"></div><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg">If we do have DWARFDie::getAttributeValue() return a DWARFFormValue in an invalid state that would solve things nicely and allow us to use the first of your syntaxes above which I found more natural:</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">x = Die.find(DW_AT_name).asString();</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">I find this to be more natural. DWARFFormValue can be default constructed and it has a zero’ed form value which is what can make it be invalid. </div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">If you are ok with this I can start a patch that does this. </div></div></blockquote><div class=""><br class=""></div><div class="">I'd like to hear from Adrian - I'm sort of on the fence about it (because I like the explicit documentation that Optional provides - so there's less chance of accidentally using DWARFFormValue without checking if it's 'set' or being confused about which APIs are traficing in possibly unset DWARFFormValues and which are not) & I know he's been trying to Optional-ify more APIs so might have some thoughts about this direction.<br class=""><br class="">I suspect it's probably going to be best/easier to use the non-member option: asString(Die.find(DW_AT_name))</div><div class=""> </div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Menlo-Regular; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="gmail_msg" style="word-wrap: break-word;"><div class="gmail_msg">The main goal for getting rid of the DWARFDie::getAttributeValueAs(..., T Default) (the ones with default values) is so we can add a boolean argument that enables us to look through the DW_AT_abstract_origin and DW_AT_specification DIEs:</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">DWARFFormValue DWARFDie::find(dwarf::Attribute Attr, bool Recurse = false);</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">If “Recurse” is true, we would check for the attribute in the current DIE, and then look through all DW_AT_abstract_origin and DW_AT_specification Dies as well. This would remove a lot of code that is doing this manually at the moment.</div></div></blockquote><div class=""><br class=""></div><div class="">Seems like that's also a good reason to find some solution that keeps the attribute type conversion separate from the attribute lookup, then - as otherwise each of the getAttributeValueAsString/Address/etc will need that extra argument.<br class=""><br class="">Also - I'd probably recommend having a separate function, rather than adding a boolean argument in this case. Most callers, I expect, will pass a constant as that second parameter - so there's no need to turn a constant into a runtime check inside the function (& have the usual un-self-documenting code problem of boolean literals as function parameters ("find(DW_AT_name, true)" - "what does the 'true' mean?" as you read the code, etc)) . Instead having 'find' and 'find_recursively' or similar.</div><div class=""> </div></div></div></div></blockquote><br class=""></div><div>Yeah, I was unhappy with the bool. I agree with you on having a separate function for this as that would be more clear and then the second solution becomes the better option.</div><div><br class=""></div><div>Can we just have the AsString be in the llvm namespace? Or should it be a static template member of DWARFFormValue? I would hope for having in the llvm namespace to keep things short?</div><div><br class=""></div><div>Greg</div></body></html>