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

[Greg Clayton via Phabricator via llvm-commits]

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.


================
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?


================
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.


https://reviews.llvm.org/D28581