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

Greg Clayton via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 11 11:57:51 PST 2017


> On Jan 11, 2017, at 11:52 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Jan 11, 2017 at 11:46 AM Greg Clayton <clayborg at gmail.com <mailto:clayborg at gmail.com>> wrote:
> 
>> On Jan 11, 2017, at 11:33 AM, David Blaikie <dblaikie at gmail.com <mailto: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. 
> 
> 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.
> 
> I suspect it's probably going to be best/easier to use the non-member option: asString(Die.find(DW_AT_name))
>  
> 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.
> 
> 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.
> 
> 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.
>  

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.

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?

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


More information about the llvm-commits mailing list