[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 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("");

to be pretty readable/not be worth having wrappers, but I can see what you
mean. 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.

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.


>
> >
> >
> > 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/b3c56d09/attachment.html>