<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="">I have submitted a new diff to review to see how we like the approach we discussed below:<div class=""><br class=""></div><div class=""><a href="https://reviews.llvm.org/D28581" class="">https://reviews.llvm.org/D28581</a></div><div class=""><br class=""></div><div class="">Check out DWARFFormValue.h and look for the toString helper functions in there. One doesn’t take a default value and returns a Optional<const char *> and the other takes a default value and returns the default value if the value isn’t valid or isn’t a string.</div><div class=""><br class=""></div><div class="">Let me know what you think of this approach.</div><div class=""><br class=""></div><div class="">Greg Clayton</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 11, 2017, at 1:02 PM, 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 12:26 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.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;"><br class="gmail_msg">> On Jan 11, 2017, at 12:07 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">><br class="gmail_msg">><br class="gmail_msg">><br class="gmail_msg">> On Wed, Jan 11, 2017 at 12:00 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a>> wrote:<br class="gmail_msg">>><br 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:<br class="gmail_msg">>> ><br class="gmail_msg">>> ><br class="gmail_msg">>> ><br 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">>> >><br 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:<br class="gmail_msg">>> >>><br 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">>> >>> 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">>> >>> 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">>> >><br class="gmail_msg">>> >> We basically have find already in:<br class="gmail_msg">>> >><br class="gmail_msg">>> >> Optional<DWARFFormValue> DWARFDie::getAttributeValue(dwarf::Attribute);<br class="gmail_msg">>> >><br class="gmail_msg">>> >> We can easily rename it.<br class="gmail_msg">>> >><br class="gmail_msg">>> >> If find returns an optional then your first code example would need to be:<br class="gmail_msg">>> >><br class="gmail_msg">>> >> x = Die.find(DW_AT_name).getValueOr(DWARFFormValue()).asString();<br class="gmail_msg">>> >><br class="gmail_msg">>> >> The second form could be made to work but it doesn’t look as natural IMHO.<br class="gmail_msg">>> >><br 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.<br class="gmail_msg">>> ><br 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">>> No matter which version we end up using I would expect any heavy users of this to re-wrap it in something more specialized again so<br class="gmail_msg">>><br class="gmail_msg">>> x = asString(Die.find(DW_AT_name).getValueOr(DWARFFormValue()));<br class="gmail_msg">>><br class="gmail_msg">>> becomes<br class="gmail_msg">>><br class="gmail_msg">>> x = getAttrAsOr<StringRef>(Die, DW_AT_name, "")<br class="gmail_msg">><br class="gmail_msg">> Not sure I follow. I would figure:<br class="gmail_msg">><br class="gmail_msg">> x = asString(Die.find(DW_AT_name)).getValueOr("");<br class="gmail_msg">><br class="gmail_msg"><br class="gmail_msg">Ok. That is different from how I understood the proposal, since asString here takes and returns an Optional. This also answers my other question, since we then can do getValueOr to unwrap the optional result of asString.<br class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg">> to be pretty readable/not be worth having wrappers, but I can see what you mean.<br class="gmail_msg"><br class="gmail_msg">It's not awesome :-)<br class="gmail_msg">The order of execution/reading is:<br class="gmail_msg"> <span class="Apple-converted-space"> </span>2 1 3<br class="gmail_msg"> <span class="Apple-converted-space"> </span>asString(Die.find(DW_AT_name)).getValueOr("");<br class="gmail_msg"></blockquote><div class=""><br class=""></div><div class="">It's certainly not uncommon to mix member and non-member function calls in expressions... </div><div 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;">Sure it's readable, but it takes a second look to correctly parse it mentally.<br class="gmail_msg"></blockquote><div class=""><br class=""></div><div class="">Yep. Alternatively, if we had unique names for these functions (perhaps put them in a 'dwarf' namespace under llvm) we could potentially have:<br class=""><br class="">Optional<const char*> toString(const DWARFFormValue&)<br class=""><br class="">Die.find(DW_AT_name).apply(toString).getValueOr("");<br class=""><br class="">perhaps (adding an Optional<T>::apply(Functor) function - but it'd have to collapse Optionals on the return value). It'd be nicer because toString wouldn't have to support an Optional parameter, but would add some complexity to Optional (& really 'apply' should probably be a free function since it's not in std::optional - which would defeat some of the point of the readability improvements).</div><div 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;"><br class="gmail_msg">> I'm not sure if there would be a nice way to keep the orthogonality (so that asString/asAddress/etc didn't all need to support/reimplement default handling) while making it much more terse. It seems like there are 3 different things happening there.<br class="gmail_msg"><br class="gmail_msg">Would it make sense to provide both variants?<br class="gmail_msg">I think it would be fine to compose the terse variant out of the generic orthogonal building blocks.<br class="gmail_msg"></blockquote><div class=""><br class=""></div><div class="">We certainly could. It wasn't clear to me if it was worth adding the wrappers for each of these. Whether shaving off a few characters at the call sites helped that much.</div><div 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;"><br class="gmail_msg">><br class="gmail_msg">> But, yes, if it comes up often enough to be annoyingly verbose - sacrificing the orthogonality (by creating/paying in the form of duplication) for convenience by writing asFoo functions that handle the defaulting - asString(Die.find(DW_AT_name), "") - same as we had before, except now the conversion is orthogonal to the lookup so adding "bool recurse"/"find_recursively" doesn't explode the API surface area along another axis.<br class="gmail_msg">><br class="gmail_msg">>> to reclaim some readability again. From that point of view I don't think the decision of where asString lives is very important. I am wondering how to pass any default value other than an empty value into getValueOr(DWARFFormValue Default)? Is it easy to construct an on-the-fly DWARFFormValue for e.g., a string?<br class="gmail_msg">><br class="gmail_msg">> I don't think we'd want people to construct DWARFFormValues to represent values that weren't present in the DWARF as a main/common means of handling situations like this. That seems to be the wrong layer - I think at the point you want an appropriately typed value, then you collapse the optionality and values pace into that type if you want to, until then it remains Optional.<br class="gmail_msg">><br class="gmail_msg"><br class="gmail_msg">Yes, I think I was misguided by the<br class="gmail_msg"> <span class="Apple-converted-space"> </span>asString(Die.find(DW_AT_name).getValueOr(DWARFFormValue()))<br class="gmail_msg">example.<br class="gmail_msg"><br class="gmail_msg">-- adrian<br class="gmail_msg">>><br class="gmail_msg">>> ><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.<br class="gmail_msg">>><br class="gmail_msg">>> I think the answer to this depends on my previous question.<br class="gmail_msg">>> -- adrian<br class="gmail_msg">></blockquote></div></div></div></blockquote></div><br class=""></div></body></html>