grimar added inline comments.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFAttribute.h:23
+ DWARFAttribute(dw_attr_t attr, dw_form_t form,
+ DWARFFormValue::ValueType value)
+ : m_attr(attr), m_form(form), m_value(value) {}
----------------
clayborg wrote:
> Do we need to use a "DWARFFormValue::ValueType" here? Right now we only need
> a int64_t and DWARFFormValue::ValueType is larger than that. It does future
> proof us a bit and there aren't all that many abbreviations, even in a large
> DWARF file. Just thinking out loud here
My thoughts to use `DWARFFormValue::ValueType` were that it seems a bit more
generic,
as standard can add other kinds of values rather than `int64_t` perhaps in
future.
And it seems to be a bit cleaner because `DWARFFormValue::ValueType` is already
existent class representing the form value while `int64_t` would be kind of
exception/special case.
I agree that for now, it is a bit excessive to have `DWARFFormValue::ValueType`
here though.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:630-631
DumpAttribute(dwarf2Data, cu, debug_info_data, &offset, s, attr,
- form);
+ form_value.Form());
}
----------------
clayborg wrote:
> DumpAttribute will need to take the full form_value in order to dump
> DW_FORM_implicit_const forms correctly. Change DumpAttribute to take a
> "form_value"
Done. I had to make it non-const `DWARFFormValue &form_value` because
`ExtractValue` call inside `DumpAttribute` is not const.
As an alternative, we could probably use pass it by value here. But since there
is a only one `DumpAttribute` call, I think its ok.
https://reviews.llvm.org/D52689
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
