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

[David Blaikie via llvm-commits]

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)



On Wed, Jan 11, 2017 at 10:19 AM Greg Clayton <clayborg at gmail.com> wrote:

> On Jan 11, 2017, at 10:01 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Jan 11, 2017 at 9:53 AM Greg Clayton via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> clayborg added a comment.
>
> I will leave the tests using getValueOr() for now as I would like it to
> fail nicely so we can get a complete snapshot of what is failing instead of
> crashing on any failure and not knowing how many others tests would fail.
>
>
> Fair enough - another tradeoff to be aware of there is that by collapsing
> the value-or-no-value states with getValueOr (or similarly with the
> previous code) it makes the failures harder to understand in some ways - if
> the EXPECT fails and the value is 0, is it because the value wasn't
> present, or because the value was present but was 0?
>
> All good - mostly I mentioned it because the lines seemed to be egtting a
> bit long/verbose and figured it might also make the tests easier to follow
> by removing some clutter.
>
>
>
> The length of the DWARFDie getAttributeValueAs functions, which were
> ported from DWARFDebugInfoEntry, was a patch I was thinking about. Does
> anyone have an objection to shortening all of the
> DWARFDie::getAttributeValueAsXXX() calls to have shorter names?
>
> I was thinking:
>
> From the current:
>
>   Optional<DWARFFormValue> getAttributeValue(dwarf::Attribute Attr) const;
>   const char *getAttributeValueAsString(dwarf::Attribute Attr, const char
> *FailValue) const;
>   Optional<uint64_t> getAttributeValueAsAddress(dwarf::Attribute Attr)
> const;
>   Optional<int64_t> getAttributeValueAsSignedConstant(dwarf::Attribute
> Attr) const;
>   Optional<uint64_t> getAttributeValueAsUnsignedConstant(dwarf::Attribute
> Attr) const;
>   Optional<uint64_t> getAttributeValueAsReference(dwarf::Attribute Attr)
> const;
>   Optional<uint64_t> getAttributeValueAsSectionOffset(dwarf::Attribute
> Attr) const;
>   DWARFDie getAttributeValueAsReferencedDie(dwarf::Attribute Attr) const;
>
> It would be nice to change “getAttributeValue” to “getAttr” and remove the
> “Constant” from UnsignedConstant” and SignedConstant”:
>
> The shorter versions would look like:
>
>   Optional<DWARFFormValue> getAttr(dwarf::Attribute Attr) const;
>   const char *getAttrAsString(dwarf::Attribute Attr, const char
> *FailValue) const;
>   Optional<uint64_t> getAttrAsAddress(dwarf::Attribute Attr) const;
>   Optional<int64_t> getAttrAsSigned(dwarf::Attribute Attr) const;
>   Optional<uint64_t> getAttrAsUnsigned(dwarf::Attribute Attr) const;
>   Optional<uint64_t> getAttrAsReference(dwarf::Attribute Attr) const;
>   Optional<uint64_t> getAttrAsSectionOffset(dwarf::Attribute Attr) const;
>   DWARFDie getAttrAsDie(dwarf::Attribute Attr) const;
>
> This would help a lot of the DWARF code out. Any thoughts or objections?
>
>
>
> I will read up on EXPECT_* and make a separate change for switching arg
> order if needed.
>
>
> Cool cool - thanks! The order of arguments catches me up every time.
>
>
>
>
> https://reviews.llvm.org/D28569
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170111/a06e8e05/attachment.html>