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

[David Blaikie via llvm-commits]

On Wed, Jan 11, 2017 at 11:46 AM Greg Clayton <clayborg at gmail.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.
>
> 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.


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