[PATCH] D28569: Remove all variants of DWARFDie::getAttributeValueAs...() that had parameters that specified default values.

[Greg Clayton via llvm-commits]

I’ll draw something up using string values only and post a diff and we can debate how the code looks and which approach works best. I’ll do this by adding the functionality and writing a quick unit test only (not convert all existing things over) so it will be easy to change direction. Let me know if anyone has any objections.

Greg

> On Jan 11, 2017, at 12:26 PM, Adrian Prantl <aprantl at apple.com> wrote:
> 
>> 
>> On Jan 11, 2017, at 12:07 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>> 
>> On Wed, Jan 11, 2017 at 12:00 PM Adrian Prantl <aprantl at apple.com> wrote:
>>> 
>>>> On Jan 11, 2017, at 11:33 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On Wed, Jan 11, 2017 at 11:08 AM Greg Clayton <clayborg at gmail.com> wrote:
>>>>> 
>>>>>> On Jan 11, 2017, at 10:36 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>> 
>>>>>> 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.
>>>>>> 
>>>>>> 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.
>>>>>> 
>>>>>> 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?)
>>>>>> 
>>>>>> So code like this, perhaps:
>>>>>> 
>>>>>>  x = Die.find(DW_AT_name).asString();
>>>>>> 
>>>>>> or perhaps:
>>>>>> 
>>>>>>  x = asString(Die.find(DW_AT_name));
>>>>>> 
>>>>>> (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)
>>>>>> 
>>>>> 
>>>>> We basically have find already in:
>>>>> 
>>>>> Optional<DWARFFormValue> DWARFDie::getAttributeValue(dwarf::Attribute);
>>>>> 
>>>>> We can easily rename it.
>>>>> 
>>>>> If find returns an optional then your first code example would need to be:
>>>>> 
>>>>> x = Die.find(DW_AT_name).getValueOr(DWARFFormValue()).asString();
>>>>> 
>>>>> The second form could be made to work but it doesn’t look as natural IMHO.
>>>>> 
>>>>> 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.
>>>> 
>>>> 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.
>>>> 
>>>> But would be good to hear from Adrian too.
>>> 
>>> 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
>>> 
>>>  x = asString(Die.find(DW_AT_name).getValueOr(DWARFFormValue()));
>>> 
>>> becomes
>>> 
>>>  x = getAttrAsOr<StringRef>(Die, DW_AT_name, "")
>> 
>> Not sure I follow. I would figure:
>> 
>>  x = asString(Die.find(DW_AT_name)).getValueOr("");
>> 
> 
> 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.
> 
> 
>> to be pretty readable/not be worth having wrappers, but I can see what you mean.
> 
> It's not awesome :-)
> The order of execution/reading is: 
>  2            1                 3  
>  asString(Die.find(DW_AT_name)).getValueOr("");
> Sure it's readable, but it takes a second look to correctly parse it mentally.
> 
>> 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.
> 
> Would it make sense to provide both variants?
> I think it would be fine to compose the terse variant out of the generic orthogonal building blocks. 
> 
>> 
>> 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.
>> 
>>> 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?
>> 
>> 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.
>> 
> 
> Yes, I think I was misguided by the 
>  asString(Die.find(DW_AT_name).getValueOr(DWARFFormValue()))
> example.
> 
> -- adrian
>>> 
>>>> 
>>>> 
>>>> 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.
>>> 
>>> I think the answer to this depends on my previous question.
>>> -- adrian

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170111/b7f46ab4/attachment.html>