[Lldb-commits] [PATCH] D52689: [LLDB] - Add support for DW_FORM_implicit_const.

[George Rimar via Phabricator via lldb-commits]

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