[PATCH] D28581: Change DWARFDie::getAttributeValue() to DWARFDie::find(), add DWARFDie::findRecurse() and dwarf::toString() helper functions.

[David Blaikie via llvm-commits]

On Wed, Jan 11, 2017 at 3:29 PM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> clayborg added inline comments.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:186-187
> +  /// or Default if the value wasn't valid or wasn't a string value.
> +  inline const char*
> +  toString(const Optional<DWARFFormValue>& V, const char *Default) {
> +    if (V) {
> ----------------
> Not sure if we need this one, but I wanted to add it just in case so we
> can see that it would work. I would probably save a few bytes on a line.
> Like things in the unit tests currently is:
>
> ```
>   EXPECT_EQ(DieDG.getAttributeValueAsUnsignedConstant(Attr_DW_FORM_data1)
>                 .getValueOr(0),
>             Data1);
> ```
>
> And it would become either:
> ```
>   EXPECT_EQ(Data1,
> dwarf::ToUnsigned(DieDG.find(Attr_DW_FORM_data1)).getValueOr(0));
> ```
>
> Or:
>
> ```
>   EXPECT_EQ(Data1, dwarf::ToUnsigned(DieDG.find(Attr_DW_FORM_data1), 0));
> ```
>
> I kind of like being able to specify the default.
>

I'm still a bit more interested in having shorter/more independent tests
that fail sooner (ASSERT rather than EXPECT) rather than having later
pieces of test code using things like default values to continue forward
progress when a previous check failed.

No worries, though - carry on as works best for you, defaults, etc.


>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:147
> +Optional<DWARFFormValue>
> +DWARFDie::findRecurse(dwarf::Attribute Attr) const {
> +  if (!isValid())
> ----------------
> dblaikie wrote:
> > probably 'findRecursively' (open to other ideas on naming
> 'findIndirectly' seems like it sort of describes things better - but it's
> not /always/ indirect... meh)
> I am open to ideas as well. findRecursively sound fine to me. Other ideas:
> - findAll
> - recursiveFind
>
> Or maybe we can rename "find" to "get" and the "get" function would only
> look in the current DIE, while "find" would go through the abs or spec dies?
>

I'm inclined towards 'find' because, like container's find (std::map::find)
that do work. 'get' feels like it implies O(1) that probably doesn't fail,
not an O(N) lookup that might fail.

findRecursively seems like it'll suffice. (findWithIndirection? meh)


>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:157
> +    if (auto Value = Die.findRecurse(Attr))
> +      return Value;
> +  return None;
> ----------------
> It usually will be just one that I am aware of, but I can't say for sure.
> I would rather be safe on this one if possible.
>

I'm not sure it's a matter of safety though - in the simplest case, this
could cause invalid inputs to cause LLDB to stack overflow (construct an
entity that refers to itself (or an indirect loop) via specification or
abstract_origin) - if there's no valid input that needs to be supported
that has multiple abstract_origin or specification steps, it might be
better to constrain it.

If not, then it'd be good to have tests for interesting cases like multiple
steps, loops, etc. (for the "take only one step" implementation, I wouldn't
bother testing the self-referential case - it's not interesting there,
doesn't cause new/interesting behavior, require special handling, etc)


>
>
> https://reviews.llvm.org/D28581
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170111/8274c121/attachment.html>