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

[Greg Clayton via llvm-commits]

> 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 <mailto:clayborg at gmail.com>> wrote:
> 
>> On Jan 11, 2017, at 10:36 AM, David Blaikie <dblaikie at gmail.com <mailto: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. 
> 
> 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.

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:

x = Die.find(DW_AT_name).asString();

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. 

If you are ok with this I can start a patch that does this. 

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:

DWARFFormValue DWARFDie::find(dwarf::Attribute Attr, bool Recurse = false);

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.

Greg Clayton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170111/4c52ea2e/attachment-0001.html>